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

tests: read test data directly from the source dir #78

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tomjnixon
Copy link
Member

@tomjnixon tomjnixon commented Apr 30, 2021

These used to be copied to the build dir, but they tended to get out of
date, as they were only copied during configuration.

With this, the test data directory is baked into the test executables, so they can
be ran outside of ctest easily.

This adds a function to get the full path of a test data file, so if we want
to change this again it should be a bit easier.

Fixes #77

These used to be copied to the build dir, but they tended to get out of
date, as they were only copied during configuration.

The test data directory is baked into the test executables, so they can
be ran outside of ctest easily.

This adds a function to get the full path of a test data, so if we want
to change this again it should be a bit easier.
@tomjnixon tomjnixon force-pushed the fix_test_data_copying branch from 96026c7 to b78a5d9 Compare April 30, 2021 16:15
@codecov-commenter
Copy link

Codecov Report

Merging #78 (b78a5d9) into master (146670a) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #78   +/-   ##
=======================================
  Coverage   89.51%   89.51%           
=======================================
  Files         115      115           
  Lines        5534     5534           
=======================================
  Hits         4954     4954           
  Misses        580      580           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 146670a...b78a5d9. Read the comment docs.

@rsjbailey rsjbailey self-requested a review May 5, 2021 12:59
@rsjbailey
Copy link
Contributor

This looks good, but a couple of questions

  • I think the behaviour previously was that with a messed up working dir, all the tests would fail, but now they will pass but you'll get some file output wherever you've run them from? Should we hard code the output dir as well or is that OK?
  • Previously .accepted and .received files were next to each other, which made doing a diff trivial, now you have to know where both live. Should we just output to the source dir and .gitignore the received files? Or is that a terrible idea?

@tomjnixon
Copy link
Member Author

tomjnixon commented May 5, 2021

I think the behaviour previously was that with a messed up working dir, all the tests would fail but now they will pass but you'll get some file output wherever you've run them from?

The current implementation will sometimes write .received.xml files to the CWD (for tests which don't read xml files, but do compare them). I think this makes that worse, but it was already a problem.

Should we hard code the output dir as well or is that OK?

Personally I'm happy with this, because git makes it pretty easy to tidy up, and it doesn't generally happen since in most cases people will not run tests manually. I wouldn't be opposed to changing it, though.

Previously .accepted and .received files were next to each other, which made doing a diff trivial, now you have to know where both live. Should we just output to the source dir and .gitignore the received files? Or is that a terrible idea?

I hadn't considered that. Other solutions would be to have FileComparator write the accepted file to the build dir on test failure(though then if you want to edit it, you might edit the wrong one), or to print the both path names on failure (unfortunately it wraps the output of describe()...).

I don't really mind writing them to the source dir though.

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

Successfully merging this pull request may close these issues.

changes to files in test_data are ignored
3 participants