Skip to content

Conversation

@Marenz
Copy link
Contributor

@Marenz Marenz commented Oct 30, 2024

  • CI: Run previously failed tests first
  • CI: Use uv backend for nox

@github-actions github-actions bot added part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) part:dispatcher labels Oct 30, 2024
@github-actions github-actions bot added the part:tests Affects the unit, integration and performance (benchmarks) tests label Oct 30, 2024
@Marenz Marenz force-pushed the failfirst branch 17 times, most recently from 74370f4 to b350319 Compare October 30, 2024 12:56
@Marenz Marenz marked this pull request as ready for review October 30, 2024 12:58
@Marenz Marenz requested review from a team as code owners October 30, 2024 12:58
Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>

# Save pytest cache only for pytest_min session
- name: Save pytest cache
if: always() && matrix.nox-session == 'pytest_min'

Choose a reason for hiding this comment

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

I think matrix.nox-session will be interpreted as a string rather than being evaluated as a variable without ${{ }}, thus the condition will be always false

Suggested change
if: always() && matrix.nox-session == 'pytest_min'
if: always() && ${{ matrix.nox-session == "pytest_min" }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's actually working, I tested this PR extensively :)

image

Choose a reason for hiding this comment

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

ok, I see it works.
I read the docs too and that confirms it too, though there are exceptions for cases that this won't work.

When you use expressions in an if conditional, you can, optionally, omit the ${{ }} expression syntax because GitHub Actions automatically evaluates the if conditional as an expression. However, this exception does not apply everywhere.

You must always use the ${{ }} expression syntax or escape with '', "", or () when the expression starts with !, since ! is reserved notation in YAML format. For example:

I've also seen in line 66 it is written if: ${{ matrix.nox-session == 'pytest_min' }} and ideally we should be consistent on how these statements are written.
IMO it feels safer to always use ${{ }}, and I agree it doesn't look good

Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

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

I would prefer if repos CI don't start to diverge, so I would definitely push for moving this to a common place like repo-config, or even better, https://github.com/frequenz-floss/gh-action-nox, and start pushing to use these GH actions more.

As for uv, I still wonder if pre-releases won't be an issue to start using it as the default. But maybe we have to suck it up and adapt to whatever uv wants, as it is orders of magnitude better than pip.

@Marenz
Copy link
Contributor Author

Marenz commented Nov 5, 2024

pushing to use these GH actions more.

What you mean "more" I looked and there isn't a single repo using them :P
Could you make one so we have a nice example of how it's used?

@llucax
Copy link
Contributor

llucax commented Nov 5, 2024

Internal repo, but there is an usage example in the action repo itself.

@Marenz Marenz closed this Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part:dispatcher part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants