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

Fix tests on windows #2803

Merged
merged 4 commits into from
Jul 11, 2024
Merged

Fix tests on windows #2803

merged 4 commits into from
Jul 11, 2024

Conversation

mquinnfd
Copy link
Contributor

Ensuring Windows test runs use the correct version

Updates the GH Action tests for Windows/Linux, aiming to produce predictable failures of tests in controlled scenarios (i.e. using the correct Windows package).

Would recommend trying this out though, I still think there's some weird behaviour of some of these process-based tests, where the Windows vs. Linux tests do not operate or fail identically, perhaps this is intended.

I suspect this can be fixed more cleanly by sorting the venv mess in the pipeline and having the windows GH Action runner be in the correct, activated venv instead of doing this, so will raise a separate issue for that.

Addresses #2796

Some testing of the breaking changes submitted in the issue by @andrewbaldwin44

image

Major

  • Update the Windows-only step to create and install a binary version of the package, to use in the tox-based tests
  • Add GH Actions matrix test value to allow front end build skipping for certain envs
  • Pass the front end build skip env var to tox for fail fast tests

Minor

  • Updated steps in Developing Locust docs to correctly invoke python, not pip3
  • Updated Developing Locust docs to be more consistent

@mquinnfd mquinnfd marked this pull request as ready for review July 11, 2024 13:49
- name: Install binary on Windows
if: ${{ matrix.os == 'windows-latest' }}
env:
SKIP_PRE_BUILD: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this is no longer needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're still doing a poetry build in here, so it will build the front end otherwise, we could of course tie this to the matrix value, so that it's picked up by any future Windows workflows (hopefully this step is removed soon though if I can figure out the venv stuff on the runner)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I thought SKIP_PRE_BUILD was already set here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps SKIP_PRE_BUILD: ${{ matrix.skip_pre_build }} would make more sense?

- { name: "3.11", python: "3.11", os: ubuntu-latest, tox: py311 }
- { name: "3.10", python: "3.10", os: ubuntu-latest, tox: py310 }
- { name: "3.9", python: "3.9", os: ubuntu-latest, tox: py39 }
- { name: "Linux", python: "3.12", os: ubuntu-latest, tox: fail_fast_test_main, skip_pre_build: "false" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

@mquinnfd I think we could skip the pre build for the "fail fast" test on Ubuntu as well right? Maybe to keep things consistent with the windows test and it would probably run faster?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So...

Sort of!?

I found that building the front end (or not) actually does affect the fail-fast tests, I could probably find an example from earlier - some things give a totally different response code though (200 vs 500) so my current understanding is that the UI build is actually not optional for those tests currently 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah I see and then I guess we just skip those tests if we're running on Windows


# install the dynamic versioning plugin for poetry
$ pip3 -m poetry self add "poetry-dynamic-versioning[plugin]"
$ python -m poetry self add "poetry-dynamic-versioning[plugin]"
Copy link
Collaborator

Choose a reason for hiding this comment

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

What will happen if this is not present? Can we make it optional, just to make the install easier for people? (maybe not now)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In terms of the plugin?

The default versioning will be used, which is not that big a deal really, we could omit this from this section of the docs if it's easier 👍

You'll just end up with something like 0.0.1dev1 or similar as the package version, not a deal breaker if you're just wanting to build a dev version of the project

What do you think?

@cyberw
Copy link
Collaborator

cyberw commented Jul 11, 2024

I'm thinking maybe using poetry ... instead of python -m poetry ... might help with readability in docs and GH actions?

Some people might have issues with poetry not having been placed in their path, but pip would/should warn them about that when poetry is installed...

Oh and same thing with pip3, just use pip. If someone still has pip pointing to pip2, well, that's just sad :)

@mquinnfd
Copy link
Contributor Author

I'm thinking maybe using poetry ... instead of python -m poetry ... might help with readability in docs and GH actions?

Some people might have issues with poetry not having been placed in their path, but pip would/should warn them about that when poetry is installed...

Oh and same thing with pip3, just use pip. If someone still has pip pointing to pip2, well, that's just sad :)

Yeah to be fair, I think poetry want you to install this as a binary package, a bit like yarn or something else - we should be able to reduce that to poetry ... commands

We're basically doing the manual install listed here, mostly because I assume people will have access to pip and it avoids either running a shell script (and dealing with OS binaries) and it avoids depending on pipx (which is not in itself a bad idea)

@mquinnfd
Copy link
Contributor Author

I'm thinking maybe using poetry ... instead of python -m poetry ... might help with readability in docs and GH actions?

Some people might have issues with poetry not having been placed in their path, but pip would/should warn them about that when poetry is installed...

Oh and same thing with pip3, just use pip. If someone still has pip pointing to pip2, well, that's just sad :)

We could also just link to the poetry install docs, as a prerequisite 🤔

@cyberw
Copy link
Collaborator

cyberw commented Jul 11, 2024

This is good enough for now I think. OK to merge?

@mquinnfd
Copy link
Contributor Author

mquinnfd commented Jul 11, 2024

This is good enough for now I think. OK to merge?

I think it's definitely an improvement over what's in master currently, I have no problems with the merge as it stands, I'll raise some Issues for any straggling issues

@cyberw cyberw merged commit e33b413 into locustio:master Jul 11, 2024
14 checks passed
@andrewbaldwin44
Copy link
Collaborator

Thanks so much for taking care of this @mquinnfd!

apereocas-bot referenced this pull request in apereo/cas Aug 1, 2024
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [locust](https://locust.io/) ([source](https://togithub.com/locustio/locust)) | `==2.29.1` -> `==2.30.0` | [![age](https://developer.mend.io/api/mc/badges/age/pypi/locust/2.30.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/pypi/locust/2.30.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/pypi/locust/2.29.1/2.30.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/pypi/locust/2.29.1/2.30.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) |

---

### Release Notes

<details>
<summary>locustio/locust (locust)</summary>

### [`v2.30.0`](https://togithub.com/locustio/locust/releases/tag/2.30.0)

[Compare Source](https://togithub.com/locustio/locust/compare/2.29.1...2.30.0)

#### What's Changed

-   FastHttpSession: Enable passing json as a positional argument for post() and stop converting response times to int by [@&#8203;tdadela](https://togithub.com/tdadela) in [https://github.com/locustio/locust/pull/2772](https://togithub.com/locustio/locust/pull/2772)
-   Remove Line Chart Default Zoom by [@&#8203;andrewbaldwin44](https://togithub.com/andrewbaldwin44) in [https://github.com/locustio/locust/pull/2774](https://togithub.com/locustio/locust/pull/2774)
-   FastHttpSession requests typing  by [@&#8203;tdadela](https://togithub.com/tdadela) in [https://github.com/locustio/locust/pull/2775](https://togithub.com/locustio/locust/pull/2775)
-   new events for heartbeat and usage monitor by [@&#8203;mgor](https://togithub.com/mgor) in [https://github.com/locustio/locust/pull/2777](https://togithub.com/locustio/locust/pull/2777)
-   dispatch benchmark test improvements by [@&#8203;tdadela](https://togithub.com/tdadela) in [https://github.com/locustio/locust/pull/2780](https://togithub.com/locustio/locust/pull/2780)
-   SequentialTaskSet: Allow weighted tasks and dict in .tasks by [@&#8203;bakhtos](https://togithub.com/bakhtos) in [https://github.com/locustio/locust/pull/2742](https://togithub.com/locustio/locust/pull/2742)
-   Fix StatsEntry docstring by [@&#8203;tdadela](https://togithub.com/tdadela) in [https://github.com/locustio/locust/pull/2784](https://togithub.com/locustio/locust/pull/2784)
-   Implement Poetry build system (mainly so we don't have to commit dynamically generated front end bundles to git) by [@&#8203;mquinnfd](https://togithub.com/mquinnfd) in [https://github.com/locustio/locust/pull/2725](https://togithub.com/locustio/locust/pull/2725)
-   Typing: strict optional in dispatch.py by [@&#8203;tdadela](https://togithub.com/tdadela) in [https://github.com/locustio/locust/pull/2779](https://togithub.com/locustio/locust/pull/2779)
-   Correctly set version from Poetry in published builds by [@&#8203;mquinnfd](https://togithub.com/mquinnfd) in [https://github.com/locustio/locust/pull/2791](https://togithub.com/locustio/locust/pull/2791)
-   Fix Extend Webui Example by [@&#8203;andrewbaldwin44](https://togithub.com/andrewbaldwin44) in [https://github.com/locustio/locust/pull/2800](https://togithub.com/locustio/locust/pull/2800)
-   Provide warning for local installs where yarn is not present by [@&#8203;mquinnfd](https://togithub.com/mquinnfd) in [https://github.com/locustio/locust/pull/2801](https://togithub.com/locustio/locust/pull/2801)
-   Fix tests on windows by [@&#8203;mquinnfd](https://togithub.com/mquinnfd) in [https://github.com/locustio/locust/pull/2803](https://togithub.com/locustio/locust/pull/2803)
-   Add example of a bottlenecked server and use that test to make a new graph for the docs by [@&#8203;cyberw](https://togithub.com/cyberw) in [https://github.com/locustio/locust/pull/2805](https://togithub.com/locustio/locust/pull/2805)
-   Replace total avg response time with 50 percentile (avg was broken) by [@&#8203;andrewbaldwin44](https://togithub.com/andrewbaldwin44) in [https://github.com/locustio/locust/pull/2806](https://togithub.com/locustio/locust/pull/2806)
-   Avoid deadlock in gevent/urllib3 connection pool (fixes occasional worker heartbeat timeouts) by [@&#8203;tdadela](https://togithub.com/tdadela) in [https://github.com/locustio/locust/pull/2813](https://togithub.com/locustio/locust/pull/2813)
-   Fix Dockerfile style warning by [@&#8203;mehrdadbn9](https://togithub.com/mehrdadbn9) in [https://github.com/locustio/locust/pull/2814](https://togithub.com/locustio/locust/pull/2814)
-   Fix pypy gc.freeze() AttributeError by [@&#8203;jimoleary](https://togithub.com/jimoleary) in [https://github.com/locustio/locust/pull/2819](https://togithub.com/locustio/locust/pull/2819)
-   Update poetry windows tests by [@&#8203;mquinnfd](https://togithub.com/mquinnfd) in [https://github.com/locustio/locust/pull/2821](https://togithub.com/locustio/locust/pull/2821)

#### New Contributors

-   [@&#8203;bakhtos](https://togithub.com/bakhtos) made their first contribution in [https://github.com/locustio/locust/pull/2742](https://togithub.com/locustio/locust/pull/2742)
-   [@&#8203;mquinnfd](https://togithub.com/mquinnfd) made their first contribution in [https://github.com/locustio/locust/pull/2725](https://togithub.com/locustio/locust/pull/2725)
-   [@&#8203;mehrdadbn9](https://togithub.com/mehrdadbn9) made their first contribution in [https://github.com/locustio/locust/pull/2814](https://togithub.com/locustio/locust/pull/2814)
-   [@&#8203;jimoleary](https://togithub.com/jimoleary) made their first contribution in [https://github.com/locustio/locust/pull/2819](https://togithub.com/locustio/locust/pull/2819)

**Full Changelog**: locustio/locust@2.29.1...2.30.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "after 10pm every weekday,before 6am every weekday" (UTC), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR was generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View the [repository job log](https://developer.mend.io/github/apereo/cas).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40NDAuNyIsInVwZGF0ZWRJblZlciI6IjM3LjQ0MC43IiwidGFyZ2V0QnJhbmNoIjoibWFzdGVyIiwibGFiZWxzIjpbIkJvdCIsIlJlbm92YXRlIl19-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants