Skip to content

BUG: fixup amrvac sample data file names in tests #3317

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

Closed
wants to merge 6 commits into from

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented Jun 1, 2021

PR Summary

fix #3315
substitutions were done semi-automatically with the apply-subs CLI, using this input file:

{
    "bw_cartesian_3d/output0001.dat": "amrvac/bw_3d0000.dat",
    "bw_cylindrical_3d/output0001.dat": "amrvac/bw_cylindrical_3D0000.dat",
    "bw_polar_2d/output0001.dat": "amrvac/bw_polar_2D0000.dat",
    "bw_spherical_2d/output0001.dat": "amrvac/bw_2d0000.dat",
    "kh2d/output0001.dat": "amrvac/kh_2d0000.dat",
    "kh3d/output0001.dat": "amrvac/kh_3D0000.dat",
    "riemann1d/output0001.dat": "amrvac/R_1d0005.dat"
}

and using the following command

git ls-files | egrep '.py$' | xargs apply-subs -s subs.json --inplace

@neutrinoceros neutrinoceros added tests: running tests Issues with the test setup bug labels Jun 1, 2021
@neutrinoceros neutrinoceros requested a review from Xarthisius June 1, 2021 20:16
@neutrinoceros
Copy link
Member Author

neutrinoceros commented Jun 2, 2021

@Xarthisius we may have a problem here. Answer tests are failing on Jenkins. This is expected, since some files changed names, but the issue is that only the filename (not the containing directory) seems to be used to name the storage file, while several of the new files are named output0001.dat, so I don't think we can avoid collisions here

There is no old answer available.
output0001.dat : GridHierarchy_output0001_dat_all

Is there any pre-existing mechanism to help us here ? I'm thinking we'd need to change the naming convention for those storage files, but that would imply changing ALL of them, probably, which seems a little too much.

@neutrinoceros
Copy link
Member Author

@yt-fido test this please

@Xarthisius
Copy link
Member

For now you can split them into separate answer files. There's no rule saying it has to be one file per frontend. Use as many as you need. The issue will go away with pytest migration.

@neutrinoceros
Copy link
Member Author

Nice. I think it's safe to drop the old ones completely so I'm going to replace the old file. I hope that's ok !

@Xarthisius
Copy link
Member

Xarthisius commented Jun 3, 2021

Nice. I think it's safe to drop the old ones completely so I'm going to replace the old file. I hope that's ok !

That doesn't solve the issue that all datasets have the same ds_name, which will conflict in the answer file. When I said multiple answer files for overlapping names I meant something like this:

diff --git a/tests/tests.yaml b/tests/tests.yaml
index 6a194a3d8..ffefa8b5d 100644
--- a/tests/tests.yaml
+++ b/tests/tests.yaml
@@ -3,9 +3,13 @@ answer_tests:
   local_art_003: # PR 3081, 3101
     - yt/frontends/art/tests/test_outputs.py:test_d9p
 
-  local_amrvac_008: # PR 2945
+  local_amrvac_something_001: # PR 2945
     - yt/frontends/amrvac/tests/test_outputs.py:test_domain_size
+
+  local_amrvac_bw_polar_001: # PR 2945
     - yt/frontends/amrvac/tests/test_outputs.py:test_bw_polar_2d
+
+  local_amrvac_bw_3d_001: # PR 2945
     - yt/frontends/amrvac/tests/test_outputs.py:test_blastwave_cartesian_3D

@neutrinoceros
Copy link
Member Author

oh that's right, thanks for your quick reply !

@neutrinoceros neutrinoceros marked this pull request as draft August 31, 2021 09:27
@matthewturk matthewturk changed the title ci: fixup amrvac sample data file names in tests CI: fixup amrvac sample data file names in tests Oct 9, 2021
@neutrinoceros neutrinoceros changed the title CI: fixup amrvac sample data file names in tests BUG: fixup amrvac sample data file names in tests Oct 14, 2021
@neutrinoceros neutrinoceros reopened this Oct 22, 2021
@neutrinoceros
Copy link
Member Author

@yt-fido test this please

@neutrinoceros
Copy link
Member Author

@yt-fido test this please

@neutrinoceros
Copy link
Member Author

I forgot to review this and now the logs are gone...
@yt-fido test this please

@neutrinoceros
Copy link
Member Author

@Xarthisius I guess I must have done something wrong here, possibly corrupted my own answers... any chance you could take a look ?

@neutrinoceros neutrinoceros deleted the hotfix_3315 branch January 21, 2022 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug tests: running tests Issues with the test setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI: update amrvac tests with new datafiles
2 participants