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

Going from 0.15.2 to 0.15.4 broke our tests #354

Open
cafkafk opened this issue Jul 25, 2024 · 5 comments
Open

Going from 0.15.2 to 0.15.4 broke our tests #354

cafkafk opened this issue Jul 25, 2024 · 5 comments
Labels
A-trycmd Area: trycmd package bug Not as expected

Comments

@cafkafk
Copy link

cafkafk commented Jul 25, 2024

See eza-community/eza#1005

For testing, normally, we run tests with just itest inside of our repo, which requires nix.

Example of breakage:

eza> Testing tests/ptests/ptest_a689ab7558716dda.toml ... failed
eza> Exit: success
eza> ---- expected: stdout
eza> ++++ actual:   stdout
eza>    1      - 8;;file:///build/source/tests/test_dir/git/git8;;/
eza>    2      - 8;;file:///build/source/tests/test_dir/grid/grid8;;/
eza>    3      - 8;;file:///build/source/tests/test_dir/group/group8;;/
eza>    4      - 8;;file:///build/source/tests/test_dir/icons/icons8;;/
eza>    5      - 8;;file:///build/source/tests/test_dir/perms/perms8;;/
eza>    6      - 8;;file:///build/source/tests/test_dir/size/size8;;/
eza>    7      - 8;;file:///build/source/tests/test_dir/specials/specials8;;/
eza>    8      - 8;;file:///build/source/tests/test_dir/symlinks/symlinks8;;/
eza>    9      - 8;;file:///build/source/tests/test_dir/time/time8;;/
eza>         1 + 8;;file://[CWD]/tests/test_dir/git/git8;;/
eza>         2 + 8;;file://[CWD]/tests/test_dir/grid/grid8;;/
eza>         3 + 8;;file://[CWD]/tests/test_dir/group/group8;;/
eza>         4 + 8;;file://[CWD]/tests/test_dir/icons/icons8;;/
eza>         5 + 8;;file://[CWD]/tests/test_dir/perms/perms8;;/
eza>         6 + 8;;file://[CWD]/tests/test_dir/size/size8;;/
eza>         7 + 8;;file://[CWD]/tests/test_dir/specials/specials8;;/
eza>         8 + 8;;file://[CWD]/tests/test_dir/symlinks/symlinks8;;/
eza>         9 + 8;;file://[CWD]/tests/test_dir/time/time8;;/
eza> stderr:
eza> 
eza> Testing tests/ptests/ptest_4974d70325cb7550.toml ... ok
eza> Testing tests/ptests/ptest_91d7b6efe549ede0.toml ... ok
eza> Testing tests/ptests/ptest_a6bbf53a066c588e.toml ... ok
eza> Testing tests/ptests/ptest_98e04e3185e9174c.toml ... ok
eza> Testing tests/ptests/ptest_4e899e9b065acc8f.toml ... ok
eza> Testing tests/ptests/ptest_5a8530dc7b091286.toml ... ok
eza> Testing tests/ptests/ptest_89146337fb6b0967.toml ... ok
eza> Testing tests/ptests/ptest_4805a91da5df26.toml ... ok
eza> Testing tests/ptests/ptest_4b30f7de50929327.toml ... failed
eza> Exit: success
eza> ---- expected: stdout
eza> ++++ actual:   stdout
eza>    1      - /build/source/tests/test_dir/git
eza>    2      - /build/source/tests/test_dir/grid
eza>    3      - /build/source/tests/test_dir/group
eza>    4      - /build/source/tests/test_dir/icons
eza>    5      - /build/source/tests/test_dir/perms
eza>    6      - /build/source/tests/test_dir/size
eza>    7      - /build/source/tests/test_dir/specials
eza>    8      - /build/source/tests/test_dir/symlinks
eza>    9      - /build/source/tests/test_dir/time
eza>         1 + [CWD]/tests/test_dir/git
eza>         2 + [CWD]/tests/test_dir/grid
eza>         3 + [CWD]/tests/test_dir/group
eza>         4 + [CWD]/tests/test_dir/icons
eza>         5 + [CWD]/tests/test_dir/perms
eza>         6 + [CWD]/tests/test_dir/size
eza>         7 + [CWD]/tests/test_dir/specials
eza>         8 + [CWD]/tests/test_dir/symlinks
eza>         9 + [CWD]/tests/test_dir/time
eza> stderr:
eza> 
@epage epage added bug Not as expected A-trycmd Area: trycmd package labels Jul 25, 2024
@epage
Copy link
Contributor

epage commented Jul 25, 2024

I assume you are reporting this against trycmd?

Do you have a minimal reproduction case I can work with for identifying the root cause?

@cafkafk
Copy link
Author

cafkafk commented Jul 26, 2024

I assume you are reporting this against trycmd?

Yup, I think that's reasonable consider the only change was the version bump leading to a breakage, if I need to move this issue lmk, I don't understand the snapbox internals >_>

Do you have a minimal reproduction case I can work with for identifying the root cause?

The problem is our tests rely on some nix sandboxing assumptions, and tests will completely break without having a nix environment. Entering the repository, typing nix develop, and running the just recipe just itest would maybe be the easiest, but again this does require having nix installed.

I know that's pretty convoluted, I'm sorry I don't have a simpler way to do this. It may be possible to do it without nix by manipulating timestamps, but there may be other behavior that also breaks in strange ways.

@epage
Copy link
Contributor

epage commented Jul 26, 2024

The challenge I'm having is I can't see why this is failing and without any context, the new output seems more correct. If /build/source was the CWD for the tests being run, I believe that substitution should have been happening.

I went back and audited this. All that changed in these versions was internals as we changed the underlying snapbox package. For reference, the command I ran was

$ git diff 72241ddc76e3ffbbb86e84e370492ea3c03d11cd -- .

(see also v0.15.2...trycmd-v0.15.4)

@cafkafk
Copy link
Author

cafkafk commented Sep 26, 2024

The actual issue that has occurred seems to be that /build/source isn't replaced with [CWD] when generated in the Nix sandbox, but when someone runs tests without being inside of the sandbox, i.e. normal cargo test in the repo, [CWD] is substituted seemingly correctly.

Perhaps the current working directory of the sandbox was root? Does that not get substituted for [CWD]?

I can't explicitly find something in the version bump that should be the culprit either, although it is odd it started appearing after. Right now my workaround is doing fd -e stdout -e stderr -H -t file tests.

@epage
Copy link
Contributor

epage commented Sep 26, 2024

Regarding the handling of [CWD], the most relevant changes I see are

  • We went from running output.replace(current_dir.display(), "[CWD]") to output.replace(current_dir.display(), "[CWD]").replace(current_dir.display().replace('\\', '/'), "[CWD]")
  • We changed the order redactions are processed from alphabetically sorted by what we redact to, to longest value we redact

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trycmd Area: trycmd package bug Not as expected
Projects
None yet
Development

No branches or pull requests

2 participants