-
Notifications
You must be signed in to change notification settings - Fork 166
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
Enable windows tests as github action #626
Conversation
See: https://github.com/jmartin-tech/garak/actions/runs/8823795608 for new action results. |
* rely on os.sep when call in `split()` on path string * lean in on Path `/` operator when combining with a Path object Signed-off-by: Jeffrey Martin <jemartin@nvidia.com>
* avoid hard coded executbale path by asking runtime path * guard file removal during cleanup Signed-off-by: Jeffrey Martin <jemartin@nvidia.com>
Signed-off-by: Jeffrey Martin <jemartin@nvidia.com>
Signed-off-by: Jeffrey Martin <jemartin@nvidia.com>
Signed-off-by: Jeffrey Martin <jemartin@nvidia.com>
Remove temp files manually for OS portablity in python 3.10+ `delete_on_close` for temp files is added in [3.12](python/cpython#97015) Signed-off-by: Jeffrey Martin <jemartin@nvidia.com>
Signed-off-by: Jeffrey Martin <jemartin@nvidia.com>
Signed-off-by: Jeffrey Martin <jemartin@nvidia.com>
2000303
to
5283c3a
Compare
Does this commit us to a change of policy, from only supporting Linux, to supporting both Linux and Windows? |
.github/workflows/tests.yml
Outdated
run: | | ||
python -m pip install --upgrade pip | ||
cd ecoji-py | ||
echo "mitigate" > README.md |
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.
nice
I would not say this commits the project to anything, this aligns the codebase for cross-platform testing at this time. |
I think it's really cool. I want to be sure we can still get useful signal from testing workflows even if the windows tests are failing, as long as the Linux ones are passing - e.g. for new features. Is there a way we can do that? |
The tests added here would only raise flags if unit tests are added that are not windows compatible. The github action could be removed for now and implemented in some other automation pattern later. Based on the current state of the code, I would suggest Windows could be documented as not officially supported however best efforts to enable compatibility will be made. There are a few different levels of resolution we could go with, off the top of my head I can think of 4 options:
|
"if unit tests are added that are not windows compatible" is a nice turn of phrase! :) Thanks for these, notes follow:
Not immediately keen on windows compatibility affecting dev pace - or at least, not automatically affecting dev pace. I want devs to be able to prototype without having a windows vm, and I want us to get a meaningful pass/fail signal from gh automated test; these desiderata seem in conflict with this requirement
Nice. In the shortest term, skipping new tests for non-linux works, though this feels like it builds in some technical debt: one needs to know to update tests to match os support policy as that changes
Agree re: potentially convoluted implementation, but this otherwise sounds relatively slick, decoupling information about different OS test pass status. It might be inconvenient to not find out about win bugs on a PR until it gets merged - but then again, if folks care about this, the requester can run the test under win, or we can also just make passing win tests required (i.e. change policy)
Non-automated testing with an optional action works for me. The requirement that we need to be intentional about this approach, and that we already are intentional about win support, seems like a good alignment to me. |
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.
really nice! thanks for the tmp
, os
, path sep, and encoding
work especially. making encoding
s explicit has been something gradually worked into the codebase for a little while now
let's work out when the tests run, then we're gtg
Signed-off-by: Jeffrey Martin <jemartin@nvidia.com>
I have shifted windows test action to manual only for now. |
Signed-off-by: Jeffrey Martin <jemartin@nvidia.com>
9870449
to
ac17eab
Compare
Fix #622
Revisions required for portable codebase testing on WIndows / Linux / MacOS as follows:
split()
on path string/
operator when combining with a Path objectutf-8
binary
flagThe added github action steps also provide an example workaround for requirements installation related to #508