Answer test refactor #245
Answer test refactor #245
Conversation
mabruzzo
added
testing
mabruzzo
force-pushed
the
answer-test-refactor
branch
from
35e80ff to
4a9c739
Compare
|
I haven't had time to look this over properly yet, but I'm in favor of the concept of this PR. The main thing I want to say now is I don't think we need to retain the old system given that it was only recently introduced. For simplicity, I'd say let's go ahead and remove the environment variable system now. |
| ^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| To run the test-suite without any answer tests, invoke ``py.test`` with the ``--answer-skip`` flag. | ||
|
|
||
| To control the location of the directory where the test answers are save, you can invoke ``py.test`` with the ``--local-dir`` flag |
There was a problem hiding this comment.
| To control the location of the directory where the test answers are save, you can invoke ``py.test`` with the ``--local-dir`` flag | |
| To control the location of the directory where the test answers are saved, you can invoke ``py.test`` with the ``--local-dir`` flag |
| # this hook is used to add more command line flags to the pytest launcher | ||
| def pytest_addoption(parser): | ||
| parser.addoption( | ||
| "--answer-skip", |
There was a problem hiding this comment.
I would prefer to only have --answer-store since they do the same thing. Both variables are mentioned above for running tests without answer testing. I think it just adds confusion.
There was a problem hiding this comment.
Sure I can do that!
I'm just spitballing here, but I think it could still be nice (but definitely not essential) to support disabling all of the answer-tests. If we aren't worried about backwards compatability, what if we made -local-dir a required argument to run the answer tests?1 In that picture:
pytest grackle/src/pythonwouldn't run all non-answer testspytest grackle/src/python --answer-store --local-dir=path/to/answerswould store answers (and run all non-answer tests)pytest grackle/src/python --answer-storewould abort with an error.pytest grackle/src/python --local-dir=path/to/answerswould check answer-test answers (and run all non-answer tests)
To be clear, I'm definitely not wedded to this idea. I'm happy to move forward and just simply remove --answer-skip. In fact, I'll just plan to go ahead and simply remove --answer-store without any sort of replacement unless I hear that you like this alternative idea.
Footnotes
-
If we went that route, maybe we should rename
--local-dirto--answer-dir↩
There was a problem hiding this comment.
Yeah, I like this. I also really like changing --local-dir to --answer-dir. "local-dir" can be interpreted in a variety of different ways, but to me "answer-dir" can only mean "the answers are/go here." If we do this, the meaning of "will not run answer tests" needs clarifying to either mean 1) the tests won't run at all or 2) the code to generate the answers will be run, but no comparison will happen.
| """ | ||
| Tests for the local functions. | ||
| """ | ||
|
|
||
| n_input_sets = 10 | ||
| seed = 21062024 | ||
| digits = 12 | ||
| test_file = os.path.join(test_answers_dir, "local_function_tests.json") | ||
| test_file_basename = "local_function_tests.json" |
There was a problem hiding this comment.
Why are we no longer storing the json file in the answer test directory?
There was a problem hiding this comment.
We do still do put the json file in the answer-test-directory. The line that you highlighted has changed to now just specifies the basename of the test-file.
On line 192 we infer the full path to the test-file with
test_file = os.path.join(answertestspec.answer_dir, self.test_file_basename)and I replaced all references to self.test_file to now access test_file.
The key change here is that I removed the global test_answers_dir variable. Now, the answer test directory is encoded in the value returned by the answertestspec fixture. (It is accessed via answertestspec.answer_dir).
Would it help if I added an explanatory comment just before the line that you highlighted?
There was a problem hiding this comment.
Ah, thanks for clarifying. The issue was mine. I think I transposed test_file with test_file_basename in my head and could no longer find the corresponding instance of os.path.join.
I can give that a try later today. I'm all for this too (it's just going to cause all of the answer tests to fail until after we update the gold standard) |
This PR lightly refactors some of the answer test functionality to make use of pytest machinery. The key point here is that you can now pass a command line flag
--answer-storeinstead of modifying theGENERATE_PYGRACKLE_TEST_RESULTSenvironment variable. The 2 primary command line flags are named after the flags used byyt.I would ultimately like to move more towards these command line flags (and maybe even eventually remove the
GENERATE_PYGRACKLE_TEST_RESULTSvariable). But we can return to that in the future