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

Windows Action Runs Wrong Version of Locust #2796

Closed
2 tasks done
andrewbaldwin44 opened this issue Jul 8, 2024 · 10 comments
Closed
2 tasks done

Windows Action Runs Wrong Version of Locust #2796

andrewbaldwin44 opened this issue Jul 8, 2024 · 10 comments
Labels

Comments

@andrewbaldwin44
Copy link
Collaborator

andrewbaldwin44 commented Jul 8, 2024

Prerequisites

Description

When breaking the tests master...andrewbaldwin44:locust:bug/windows-action the Windows test action will not fail

image

@mquinnfd This should be poetry related, any idea?

Python version

3.12

Locust version

2.29

Operating system

Windows

@mquinnfd
Copy link
Contributor

mquinnfd commented Jul 9, 2024

Hey @andrewbaldwin44 I'll push something to replicate this, would be good to see the logs from the failing action 🧑‍💻

@mquinnfd
Copy link
Contributor

mquinnfd commented Jul 9, 2024

I think this is because...

  • For Windows builds, we added a step to build & install the locust package, to put the current version on the PATH
  • This is because poetry install did not appear to add locust to the PATH so caused tests to fail
  • Running poetry build on the Windows runner, will mean that the front end build is run
  • The version of locust used by the fail_fast tests will be the one that was build and installed as a package, not a dev install

So, in terms of what to do with this, the best way is probably to find the editable install location on the Windows runner, and add this to the PATH so the fail_fast tests can run. I avoided this initially because it might look a bit grim in the runner steps but it does make this build more predictable

We could of course skip the pre-build on Windows, meaning no front end is built, but I suppose the main issue here is that the tox behaviour is a little different on Windows, with the changes, so sorting it properly makes the most sense.

@mquinnfd
Copy link
Contributor

mquinnfd commented Jul 9, 2024

Definitely fixable I'll just need to run this on my Windows machine to check the commands work

@andrewbaldwin44
Copy link
Collaborator Author

Hey @mquinnfd, sorry about that I should have sent this link as well https://github.com/andrewbaldwin44/locust/actions/runs/9840186826/job/27163841986. The logs from the actions could have been seen on my fork but no matter :)

I think no frontend being built on Windows is not really an issue since we don't need the frontend for fail_fast right?

@mquinnfd
Copy link
Contributor

mquinnfd commented Jul 9, 2024

image
Didn't realise I could see your fork's actions, sorry

Yeah that's true - it was building this for the build step though, so tried removing it but it still doesn't fail the test.

It's a little bit weird, but I suppose the rub is that poetry build is overriding the output of poetry install here in ways which are not cool.

I tried making these in parity, technically you would expect these to function the same, but I guess not. I'll need to write up something that ensures the binary/symlink created by poetry install is available on the runner's PATH.

This probably boils down activating the venv or something 🤔

@andrewbaldwin44
Copy link
Collaborator Author

Yeah I was also thinking that activating the venv could have something to do with it, but I was thinking that if it was an issue with venv, then we should see an error such as 'locust' is not recognized as an internal or external command

@mquinnfd
Copy link
Contributor

Hey @andrewbaldwin44 to follow up on this:

I tested this code change on my Windows machine this morning - interestingly I cannot actually get the fail-fast (tests/test_main.py) to break with this change. I tried this by doing the same thing the Windows runner is doing, building and installing it so that the tests can call locust on the path.

Do you know which tests you expect to break on this change? I note that 21 tests are skipped on the nt platform, citing signal handling problems - it could be that the tests in these blocks are the ones that would fail without the web UI being set, maybe?


I tried a few ways of getting locust on the path for the tests without this build, the Windows cmd shell is just horrible for trying to do this neatly, and would probably need to either revise the virtual environments for the steps or write a batch script...

@andrewbaldwin44
Copy link
Collaborator Author

Yeah so that's actually exactly the problem :) The fact that the tests should fail and they are not failing indicates that the Locust version being used is incorrect. We were actually able to reproduce this in several ways, here's another example:

master...test-tests in this example the Windows tests are failing even when they should be passing https://github.com/locustio/locust/actions/runs/9840237867

@andrewbaldwin44
Copy link
Collaborator Author

Yeah the Windows shell is quite horrible I really don't understand how anyone is able to use Windows without WSL haha. I have a Windows VM on my side as well, I don't know as much about poetry as yourself but I will try to help if I can!

@mquinnfd
Copy link
Contributor

mquinnfd commented Aug 7, 2024

Should be fixed in #2803 @andrewbaldwin44 might be OK to close

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants