-
Notifications
You must be signed in to change notification settings - Fork 97
Add checks for a new environment variable to support running unit tests in read-only environments #2410
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
base: main
Are you sure you want to change the base?
Conversation
|
Note: I haven't added documentation or tests for this yet. Just putting the PR up so the approach can be reviewed |
| sys.stderr = self.stderrLogger | ||
|
|
||
|
|
||
| def getLogDir(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get a one-line docstring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fo sho! Wasn't sure you'd like this approach so didn't document it yet :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
|
|
||
| uniformTempsInC = [400.0, 500.0, 600.0, 700.0, 800.0, 900.0, 1200.0] | ||
| uniformTempsInC = [300.0, 400.0, 500.0, 600.0, 700.0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to merge ARMI main into this branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I pulled in clean-path then squashed the commits so the commit history would be easier to see on this branch.
I need this + clean-path together for my testing.
You can review the last 2 commits alone, or you can wait until clean-path is merged and this PR is updated with main
| from pathlib import Path | ||
| from shutil import rmtree | ||
|
|
||
| import pytest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, well, actually. We need to talk about this.
So, your team really wants to be able to import the ARMI unit tests. And they do that all over the place. And right now pytest is not part of the ARMI runtime dependencies.
But if you add this line here, then we also need to add pytest to the ARMI runtime dependencies. I'm not THRILLED about that. If you think this is super important, we can do it. I'll need a little convincing, maybe.
|
I believe your new |
What is the change? Why is it being made?
NOTE: This PR is built on top of the
clean-pathbranch via a squash commit for my own testing. Once that is merged, this PR will show a cleaner diff.This PR adds a check for an env variable in two places: During the
logsdir creation and as arootarg override inTemporaryDirectoryChanger. This is to support read-only containerization efforts. ARMI's unit tests wouldn't all be able to run with these edits, but they don't have to. The downstream tests can.SCR Information
Change Type: features
One-Sentence Rationale: ARMI is being run in ready-only containers downstream, and these changes are needed to support that.
One-line Impact on Requirements: NA
Checklist
docfolder.pyproject.toml.