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

Enable windows tests as github action #626

Merged
merged 10 commits into from
Apr 26, 2024

Conversation

jmartin-tech
Copy link
Collaborator

Fix #622

Revisions required for portable codebase testing on WIndows / Linux / MacOS as follows:

  • rely on os.sep when calling split() on path string
  • lean in on Path / operator when combining with a Path object
  • avoid hard coded executable path by asking runtime path
  • guard file removal during cleanup
  • force report digest output as utf-8
  • set encoding for all file operations without binary flag
  • portable temp file remove for 3.10+
  • add windows pytest actions for 3.10 and 3.12

The added github action steps also provide an example workaround for requirements installation related to #508

@jmartin-tech
Copy link
Collaborator Author

jmartin-tech commented Apr 24, 2024

* 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 <[email protected]>
* avoid hard coded executbale path by asking runtime path
* guard file removal during cleanup

Signed-off-by: Jeffrey Martin <[email protected]>
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 <[email protected]>
Signed-off-by: Jeffrey Martin <[email protected]>
Signed-off-by: Jeffrey Martin <[email protected]>
@leondz
Copy link
Collaborator

leondz commented Apr 25, 2024

Does this commit us to a change of policy, from only supporting Linux, to supporting both Linux and Windows?

run: |
python -m pip install --upgrade pip
cd ecoji-py
echo "mitigate" > README.md
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

@jmartin-tech
Copy link
Collaborator Author

I would not say this commits the project to anything, this aligns the codebase for cross-platform testing at this time.

@leondz
Copy link
Collaborator

leondz commented Apr 25, 2024

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?

@jmartin-tech
Copy link
Collaborator Author

jmartin-tech commented Apr 25, 2024

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:

  • require any pytest to pass in windows as well
    • new unit tests should be testing python code or project functions and mocking native code tooling
  • document support for add annotation that would be place on pytest methods for functionality not supported on windows
    • Most comprehensive approach
    • think @pytest.mark.skipif(sys.platform == "win32")
  • shift windows testing to an action that only occurs on a branch that separate automation would trigger to test only after a PR lands to main
    • A bit convoluted to implement but shifts burden of windows to a canary action that does not block PR progression
  • shift windows testing to an action without an automatic trigger that must be executed by a project maintainer or only on new tagged release generation
    • Runs the risk of rarely being validated or adding last minute findings of issues that could delay release schedules

@leondz
Copy link
Collaborator

leondz commented Apr 25, 2024

"if unit tests are added that are not windows compatible" is a nice turn of phrase! :)

Thanks for these, notes follow:

require any pytest to pass in windows as well

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

document support for add annotation that would be place on pytest methods for functionality not supported on windows

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

shift windows testing to an action that only occurs on a branch that separate automation would trigger to test only after a PR lands to main

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)

shift windows testing to an action without an automatic trigger that must be executed by a project maintainer or only on new tagged release generation

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.

Copy link
Collaborator

@leondz leondz left a 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 encodings 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

.github/workflows/tests.yml Outdated Show resolved Hide resolved
garak/probes/leakreplay.py Outdated Show resolved Hide resolved
garak/probes/leakreplay.py Outdated Show resolved Hide resolved
garak/probes/leakreplay.py Outdated Show resolved Hide resolved
garak/probes/misleading.py Show resolved Hide resolved
tests/analyze/test_analyze.py Outdated Show resolved Hide resolved
tests/test_config.py Outdated Show resolved Hide resolved
tests/test_docs.py Show resolved Hide resolved
Signed-off-by: Jeffrey Martin <[email protected]>
@jmartin-tech
Copy link
Collaborator Author

I have shifted windows test action to manual only for now.

Signed-off-by: Jeffrey Martin <[email protected]>
@leondz leondz merged commit 79c7649 into NVIDIA:main Apr 26, 2024
4 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 26, 2024
@jmartin-tech jmartin-tech deleted the fix/windows-tests branch May 2, 2024 18:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows test failures 2024-04
2 participants