Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support multiple references #47

Open
johnnychen94 opened this issue Jan 12, 2020 · 5 comments
Open

support multiple references #47

johnnychen94 opened this issue Jan 12, 2020 · 5 comments

Comments

@johnnychen94
Copy link
Member

johnnychen94 commented Jan 12, 2020

This can be useful in two cases:

  • version compatibility: with dependency version changes, the actual results might change as well. Since a simple update of reference will break tests for old dependency versions, the best strategy is to hold two references there and let the test pass as long as the actual result matches one of them (e.g., the workaround update references for FixedPointNumbers v0.7 #49 for correct the rendermode for txt images #46 doesn't consider this).
  • random behavior: to suit the case when the actual result is not deterministic (e.g., actual = first(randperm(3)))
@kimikage
Copy link

In the former case, I think we should verify to match one "specific" reference rather than "any" one.
What about adding the compatible package versions as a suffix as follows?

  • "reference.txt"
  • "reference_julia[1.4-].txt"
  • "reference_julia[1.4-]_FixedPointNumbers[0.7-0.8].txt"

In practice, I think we only need to support a limited set of rules. Complex rules are not suitable for handling within filenames. The users can still write complex logic in their test code.

@johnnychen94
Copy link
Member Author

johnnychen94 commented Aug 11, 2020

Thanks for the enlightening feedback, and the one in #66. Unfortunately, I've become exhausted to continue the enhancement of this package recently because of a series of day-to-day seminars. If things go smoothly, I'll come back and continue these in September.

In practice, I think we only need to support a limited set of rules. Complex rules are not suitable for handling within filenames. The users can still write complex logic in their test code.

This issue right now is at an awkward status that if we support this, then the regeneration workflow #52 with rm -rf test/references won't work perfectly; if there are multiple references candidates, then we could only generate one in each setting, while the others can only be restored using git (this works but is not perfect as those removed references can be outdated).

I now prefer to use more eager pre-processing rules and the by keyword more often to filter out and ignore irrelevant information. Just as I've done in https://github.com/JuliaTesting/ReferenceTests.jl/pull/66/files#diff-83b01706c3eadcbd57f11f30a83e1dacR3-R26 as you already noticed.

@kimikage
Copy link

ReferenceTests.jl can determine if each existing reference file is valid or invalid (but not sure if that is what the user intended), so it can provide a smarter method of regeneration than rm -rf workflow.

However, I don't think the concrete implementation needs to be discussed here for now.

I think providing pre-processors and their interfaces is a good idea. 👍
However, it is also essential to compare not "live" objects but human-readable "frozen" objects. As I wrote in the comment in the PR, I think that the references in reference tests should be rewritten extremely carefully and strictly.

@johnnychen94
Copy link
Member Author

However, I don't think the concrete implementation needs to be discussed here for now.

FWIW, the plan I have in mind is to first implement a match_reference function that follows the same rule as @test_reference but returns true/false as suggested in #47 and then see what can be improved after that.

Doing that could easily bring unexpected breaking changes, and that's why I want to do #66 first. :sigh:

@oxinabox
Copy link
Member

This idea is growing on me.
In oxinabox/LayeredLayouts.jl#4
there are multiple equally good solutions and depending how the solver is feeling it can come up with different images

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

No branches or pull requests

3 participants