Change to GIX_TEST_IGNORE_ARCHIVES on CI and docs to match code#1360
Conversation
The test suite checks for `GIX_TEST_IGNORE_ARCHIVES` and not for `GITOXIDE_TEST_IGNORE_ARCHIVES`. But the main test workflow `ci.yml`, as well as in the instructions in `DEVELOPMENT.md`, had given `GITOXIDE_TEST_IGNORE_ARCHIVES`. This fixes that.
Sebastian Thiel (Byron)
left a comment
Sebastian Thiel (Byron)
left a comment
There was a problem hiding this comment.
This…is a great catch!
Indeed, previously this environment variable wasn't as important for the test job, one simply wouldn't checkout git lfs. Now that these archives are integrated into the repository, they are always present though and thus the variable should be set correctly.
Now, I believe, this is once again the case.
This also means that for quite some time now, the test-scripts weren't run and thus never re-validated on unix where they are definitely expected to work.
There is no test-output as the test-runner by default captures all output, which will only be displayed in case of an error. cargo test -- --nocapture would do the trick, maybe that helps to locally see the messages you were expecting.
This replaces the old reason why it's not necessry to test locally on Linux-based systems with `GIX_TEST_IGNORE_ARCHIVES`, which has not been current for quite some time since the generated archives are no longer in LFS, with the current reason. This builds on the documentation part of GitoxideLabs#1360.
This modifies the readme and one source code file to no longer say that Git LFS is required to get generated archives, because they are checked in as regular blobs instead these days, ever since 35439de (GitoxideLabs#1176). This may simplfy onboarding since readers are no longer told they must do anything with `git lfs` to get started. It also removes the LFS step from a CI test job definition that had it, for the same reason: the generated archives are no longer stored in LFS, so the only current expected effects of that step are to make the job slightly more complicated and very slightly slower. See also 7b3104d (GitoxideLabs#1360) and c60f270.
2 participants
The test suite checks for
GIX_TEST_IGNORE_ARCHIVESand not forGITOXIDE_TEST_IGNORE_ARCHIVES. But the main test workflowci.yml, as well as in the instructions inDEVELOPMENT.md, had givenGITOXIDE_TEST_IGNORE_ARCHIVES. This fixes that.I'm unsure what the effect was in practice of this disparity. Experimentation with modified workflows in my fork has not revealed any messages indicating that generated archives are skipped, before or after this change. The explanation in
DEVELOPMENT.mdsays that they are absent on CI for Linux because they are never checked out from Git LFS, but runninggit lfs ls-filesdoesn't show anything on any systems, even after runninggit lfs installandgit lfs pull(the latter of which entailsgit lfs checkout, but I have also tried running that just in case). In addition, I think I recall your having said that archives that were formerly stored in Git LFS are now stored normally in the repository (due to limitations of Git LFS in GitHub).However, at least the documentation fix is important. In #1345 (comment) I had mentioned that I had thought I'd run the tests with
GIX_TEST_IGNORE_ARCHIVES=1before and found them to pass, yet that is not the case (#1358). I now believe what I had set was insteadGITOXIDE_TEST_IGNORE_ARCHIVES=1, based on the instructions inDEVELOPMENT.md. I have since verified again thatGIX_TEST_IGNORE_ARCHIVES=1produces the failures documented in #1358, whileGITOXIDE_TEST_IGNORE_ARCHIVES=1has no effect compared to not setting any environment variables and does not produce any failures (presumably due to not causing archives to be ignored).An alternative to these changes could be to modify the code so
GITOXIDE_TEST_IGNORE_ARCHIVESis recognized, or so that both names are recognized.I suggest reviewing this to ensure that, at least after this PR (if not before), CI tests without using generated archives in at least one job at least on Ubuntu, so that changes to shell scripts are being validated. As mentioned above, based on the (admittedly nonequivalent) CI results within my fork, I am not confident of this.