-
Notifications
You must be signed in to change notification settings - Fork 35
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
Use a run_task abstraction for eval-tests.py #393
Conversation
d8cf127
to
15adb9a
Compare
So this test was really useful in #401 rewriting eval.sh to eval.py and simplifying the task graph, as I could re-run the test to make sure I wasn't breaking things. |
Here is an example of an existing test using @pytest.mark.parametrize(
"params",
[
("mtdata", "Neulab-tedtalks_test-1-eng-rus"),
("opus", "ELRC-3075-wikipedia_health_v1"),
("flores", "dev"),
("sacrebleu", "wmt19"),
],
)
def test_basic_corpus_import(params, data_dir):
importer, dataset = params
data_dir.run_task(
f"dataset-{importer}-{dataset}-en-ru",
env={"COMPRESSION_CMD": COMPRESSION_CMD, "ARTIFACT_EXT": ARTIFACT_EXT},
)
prefix = data_dir.join(f"artifacts/{dataset}")
output_src = f"{prefix}.ru.gz"
output_trg = f"{prefix}.en.gz"
assert os.path.exists(output_src)
assert os.path.exists(output_trg)
assert len(read_lines(output_src)) > 0
assert len(read_lines(output_trg)) > 0 |
This fixes an issue where if you modify the tc.prod.yml it will break the taskcluster tests. It will also allow for tests to share the same config between runs and not have a dirty artifacts folder.
15adb9a
to
d337e1f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall! I have a minor remark about asserts. Running tasks is a great way to integrate a bit deeper and improve coverage.
Instead of testing at the script level, this tests at the run-task level, which I feel is the integration test level.
I'm keeping this as draft because it contains work from #364 that I would like to land first. I also wanted to get a CI run in. The most relevant commit here is ecd2793Edit: This is ready for review.