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

filter: Update working directory of cram tests #1133

Merged
merged 2 commits into from
Jan 28, 2023

Conversation

victorlin
Copy link
Member

@victorlin victorlin commented Jan 23, 2023

Description of proposed changes

$TMP is the test runner's temp directory, which is shared across all individual tests. This means files must be removed before running the next test to ensure a clean slate.

On the other hand, the initial working directory of each test is a directory within $TMP which is truly temporary per test file.

I updated all tests to use this initial working directory as a temporary directory. A summary of the changes:

  1. Change the working directory from $TESTDIR/../../ (tests/functional/) to the default initial working directory.
  2. Update references to files in tests/functional/filter/data, since those had relied on the parent folder as the working directory.
  3. Remove directory changes in the "setup" section, simplifying that to one command creating the AUGUR alias relative to $TESTDIR.
  4. Remove rm commands used to clean up output files, since the working directory is a per-test temporary directory.
  5. filter-metadata-sequence-strains-mismatch.t: Update the diff check to a direct check of file contents, since the resolved $TESTDIR must be matched by regex.

Note that if this approach is applied to other cram tests, issues/workarounds such as #863 and #1077 can be avoided.

Related issue(s)

N/A

Testing

What steps should be taken to test the changes you've proposed?
If you added or changed behavior in the codebase, did you update the tests, or do you need help with this?

  • Updated tests are successful in CI

Checklist

  • Add a message in CHANGES.md summarizing the changes in this PR that are end user focused. Keep headers and formatting consistent with the rest of the file. N/A, dev change

@victorlin victorlin self-assigned this Jan 23, 2023
@codecov
Copy link

codecov bot commented Jan 23, 2023

Codecov Report

Base: 68.08% // Head: 68.11% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (5d2e370) compared to base (85d2edd).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1133      +/-   ##
==========================================
+ Coverage   68.08%   68.11%   +0.02%     
==========================================
  Files          62       62              
  Lines        6690     6690              
  Branches     1641     1641              
==========================================
+ Hits         4555     4557       +2     
+ Misses       1829     1827       -2     
  Partials      306      306              
Impacted Files Coverage Δ
augur/__version__.py 100.00% <100.00%> (ø)
augur/filter/io.py 82.05% <0.00%> (+5.12%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@victorlin victorlin requested a review from a team January 23, 2023 22:10
@victorlin victorlin force-pushed the victorlin/update-cram-tmp-dir-usage branch from d2ea4f0 to 4145e1b Compare January 27, 2023 20:15
Copy link
Member

@tsibley tsibley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for this direction!

$TMP is the test runner's temp directory, which is shared across all
individual tests¹. This means files must be removed before running the
next test to ensure a clean slate.

On the other hand, the initial working directory of each test² is a
directory within $TMP which is truly temporary per test file.

I updated all tests to use this initial working directory as a temporary
directory. A summary of the changes:

1. Change the working directory from "$TESTDIR/../../"
   (tests/functional/) to the default initial working directory.
2. Update references to files in tests/functional/filter/data, since
   those had relied on the parent folder as the working directory.
3. Remove directory changes in the "setup" section, simplifying that to
   one command creating the AUGUR alias relative to $TESTDIR.
4. Remove rm commands used to clean up output files, since the working
   directory is a per-test temporary directory.
5. filter-metadata-sequence-strains-mismatch.t: Update the diff check to
   a direct check of file contents, since the resolved $TESTDIR must be
   matched as a pattern.

¹ https://github.com/brodie/cram/blob/61212ab78a88ce4a18eee4e26c89bfe086b28e78/cram/_main.py#L185
² https://github.com/brodie/cram/blob/61212ab78a88ce4a18eee4e26c89bfe086b28e78/cram/_run.py#L67-L70
The first point describes a new way to organize test files (started by
5ce415f, used in
tests/functional/curate/cram and tests/functional/filter/cram)

The second point summarizes the previous commit.
@victorlin victorlin force-pushed the victorlin/update-cram-tmp-dir-usage branch from 7f738e5 to 5d2e370 Compare January 27, 2023 23:09
@victorlin victorlin merged commit e7965fc into master Jan 28, 2023
@victorlin victorlin deleted the victorlin/update-cram-tmp-dir-usage branch January 28, 2023 00:09
joverlee521 added a commit that referenced this pull request Jan 31, 2023
joverlee521 added a commit that referenced this pull request Jun 12, 2023
joverlee521 added a commit that referenced this pull request Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

2 participants