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.

Sign up for GitHub

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

Jump to bottom

Test suite: after running a test set, throw an error if Base.DEPOT_PATH, Base.LOAD_PATH, or ENV have been modified and not restored to their original values #46602

Merged
merged 1 commit into from
Sep 6, 2022
Merged

Test suite: after running a test set, throw an error if Base.DEPOT_PATH, Base.LOAD_PATH, or ENV have been modified and not restored to their original values #46602

merged 1 commit into from
Sep 6, 2022

Conversation

DilumAluthge
Copy link
Member

@DilumAluthge DilumAluthge commented Sep 2, 2022 •
edited
Loading

No description provided.

@DilumAluthge DilumAluthge added testsystem The unit testing framework and Test stdlib ci Continuous integration labels Sep 2, 2022
@DilumAluthge DilumAluthge changed the title Test suite: if a test set mutates Base.DEPOT_PATH, make sure to restore the original Base.DEPOT_PATH at the end of the test set Test suite: save the original Base.DEPOT_PATH before running a test set, and restore it after running the test set Sep 2, 2022
@DilumAluthge DilumAluthge changed the title Test suite: save the original Base.DEPOT_PATH before running a test set, and restore it after running the test set Test suite: save the original Base.DEPOT_PATH and Base.LOAD_PATH before running a test set, and restore them after running the test set Sep 2, 2022
@DilumAluthge DilumAluthge marked this pull request as ready for review September 2, 2022 16:17
@gbaraldi
Copy link
Member

gbaraldi commented Sep 2, 2022

LGTM, whitespace check aside.

@DilumAluthge DilumAluthge force-pushed the dpa/restore-depot-path branch 2 times, most recently from 0845591 to c0daaa5 Compare September 3, 2022 14:06
@KristofferC
Copy link
Sponsor Member

Hm, why though? This feels kind of arbitrary. What about the eg the ENV or other global state.

@DilumAluthge
Copy link
Member Author

DilumAluthge commented Sep 3, 2022 •
edited
Loading

There is currently a known non-deterministic test failure in the cmdlineargs test set that is caused by other test sets that modify Base.DEPOT_PATH and do not properly restore the original value. If the cmdlineargs test set happens to run on the same worker process that previously ran one of those test sets, then the failure will occur.

So this PR only deals with the depot path and load path because those are the ones relevant to this specific cmdlineargs test failure. But I have no objection to also checking ENV or other pieces of global state.

An alternative would be to always kill the worker process after each test set, so each test set gets a brand new worker process. (But I'm not sure if that would work for the "only run on node 1" test sets.

@DilumAluthge DilumAluthge changed the title Test suite: save the original Base.DEPOT_PATH and Base.LOAD_PATH before running a test set, and restore them after running the test set Test suite: after running a test set, throw an error if Base.DEPOT_PATH, Base.LOAD_PATH, or ENV have been modified and not restored to their original values Sep 3, 2022
@DilumAluthge
Copy link
Member Author

I've added ENV to the list of global things that we check. If there are other global states that we should check, we can add those also.

@DilumAluthge
Copy link
Member Author

DilumAluthge commented Sep 3, 2022 •
edited
Loading

Depot path fix for Pkg is in JuliaLang/Pkg.jl#3191 via #46615.

@DilumAluthge
Copy link
Member Author

Depot path fixes for loading and precompile are in #46616.

@DilumAluthge
Copy link
Member Author

DilumAluthge commented Sep 3, 2022 •
edited
Loading

The ENV fixes for corelogging, env, and REPL are in #46617.

@DilumAluthge DilumAluthge force-pushed the dpa/restore-depot-path branch 3 times, most recently from d179839 to d493711 Compare September 3, 2022 22:55
@DilumAluthge DilumAluthge marked this pull request as draft September 4, 2022 03:19
@DilumAluthge
Copy link
Member Author

DilumAluthge commented Sep 4, 2022 •
edited
Loading

The ENV fix for Pkg is in JuliaLang/Pkg.jl#3192 via #46639.

@DilumAluthge DilumAluthge marked this pull request as ready for review September 5, 2022 23:56
@gbaraldi
Copy link
Member

gbaraldi commented Sep 6, 2022

I think leaving the ENV checks there is fine and hopefully this stops naught tests again. LGTM

@DilumAluthge DilumAluthge added the merge me PR is reviewed. Merge when all tests are passing label Sep 6, 2022
@DilumAluthge
Test suite: after running a test set, throw an error if `Base.DEPOT_P…
15a39da 
…ATH`, `Base.LOAD_PATH`, or `ENV` have been modified and not restored to their original values
@DilumAluthge DilumAluthge merged commit 305e2fb into master Sep 6, 2022
@DilumAluthge DilumAluthge deleted the dpa/restore-depot-path branch September 6, 2022 16:51
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Sep 6, 2022
@clouds56 clouds56 mentioned this pull request Sep 7, 2022
Closed
@vtjnash vtjnash mentioned this pull request Feb 20, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtjnash vtjnash Awaiting requested review from vtjnash

@staticfloat staticfloat Awaiting requested review from staticfloat

Assignees
No one assigned
Labels
ci Continuous integration testsystem The unit testing framework and Test stdlib
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DilumAluthge @gbaraldi @KristofferC