Skip to content

Conversation

@fingolfin
Copy link
Collaborator

This adds back one of the tests removed in #2804 but in a form that's less useless. (See #2804 (comment) for more information about this test.)

However, this will fail CI in the current fork, because the warning (about paths containing :) is actually different on Windows (were our custom code triggers) versus Unix (where a path with a : is fine, but Documenter will complain because the specified files don't actually exist).

So how should we deal with this? Some options:

  1. we could just not bother and drop this test (honestly, I think this would be fine, except that it'd drop test coverage a bit)
  2. we could extend the check which forbid filenames with a : to trigger independently of the OS
  3. we could have two different "expected texts" for this test: one for windows, one for everyone else (similar to how the test/doctests tests allow different versions of a test based on the Julia version)

Thoughts?

@mortenpi
Copy link
Member

In general, I'd prefer we didn't delete tests that exist and exercise lines that otherwise would not be covered. Minimally, they will catch things that throw, like missing variable errors etc.

As for this test.. I think that, in general, we do need to be able to have separate references per Julia version and per OS. So (3) seems like the simple approach, even though a bit annoying.

@fingolfin
Copy link
Collaborator Author

In general, I'd prefer we didn't delete tests that exist and exercise lines that otherwise would not be covered. Minimally, they will catch things that throw, like missing variable errors etc.

Counter point: by having a test that exercises a line of code but only catches the most severe cases of failures, we obscure the fact that the code in there is in fact not well-tested.

I'd rather have a proper test than an incomplete test whose failures are hidden by default.

This is why I did not just delete the tests, but instead also opened a PR to re-add it, but in a more helpful way.

As for this test.. I think that, in general, we do need to be able to have separate references per Julia version and per OS. So (3) seems like the simple approach, even though a bit annoying.

OK, that seems fair enough to me. Having the ability to do so may come in handy in other instances anyway.

I can't promise to implement this here right away, but I'll get to it eventually -- but probably after addressing #2819 (or perhaps even as part of it)

@mortenpi
Copy link
Member

I'd rather have a proper test than an incomplete test whose failures are hidden by default.

I completely agree that a better test is better 😄 I was just responding, more in general, to

  1. we could just not bother and drop this test (honestly, I think this would be fine, except that it'd drop test coverage a bit)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants