Skip to content

Fix CI check for code changed (suggested by Zach) #1410

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

Merged
merged 3 commits into from
May 2, 2025

Conversation

ameba23
Copy link
Collaborator

@ameba23 ameba23 commented Apr 30, 2025

In some cases tests were not being run despite code having been changed, due to an issue with the check as to whether code changes. Eg: https://github.com/entropyxyz/entropy-core/actions/runs/14751508488/job/41409844605?pr=1392

@cooldracula suggested this fix - making it so that all commit history is checked out, so git diff can find the commits we are referring to.

@ameba23 ameba23 requested a review from cooldracula April 30, 2025 11:29
@cooldracula
Copy link
Contributor

With the additional code added to check if this is the first commit on a branch, I think this looks good!

@ameba23
Copy link
Collaborator Author

ameba23 commented May 1, 2025

With the additional code added to check if this is the first commit on a branch, I think this looks good!

That great - on my branch the check evaluates to true when i pushed a trivial code change. But for some reason the tests are still being skipped. I feel like i must be missing something really stupid.

https://github.com/entropyxyz/entropy-core/actions/runs/14771602673/job/41472444384?pr=1392

@cooldracula
Copy link
Contributor

cooldracula commented May 1, 2025

That great - on my branch the check evaluates to true when i pushed a trivial code change. But for some reason the tests are still being skipped. I feel like i must be missing something really stupid.

The only thing I can think of, at a glance, is that the steps feel a little roundabout. The code checkout echoes to $GITHUB_ENV which is then echoed to $GITHUB_OUTPUT which is then checked in the next step. This could cause the value to be subtly different than what we expect. Could we just echo directly to $GITHUB_OUTPUT in the code-paths-changed job? I'll push a commit to this branch with what I mean.

@cooldracula
Copy link
Contributor

cooldracula commented May 1, 2025

Ah, looking at it closer, I think I see the issue with the other job. My commit above will fix it, indirectly, and so I still want to explain it here.

On line 10 of the workflow file, you set the output as:

  outputs:
      run_tests: ${{ steps.filter.outputs.changes_detected }}

And the steps are then:

     - name: Check for relevant changes
       id: filter
       run: |
         ## echo values to $GITHUB_ENV
     - name: Set output
       run: # echo env values to $GITHUB_OUTPUT

The outputs definition uses the outputs of the step with the filter ID, but that step isn't outputting anything. Instead, the output is changed in the subsequent step, which has no ID.

So another solution would be to set the filter ID on that last step, but I don't think this is ideal as, ultimately, that step is redundant and should be removed. It makes more sense, to me, to have our git diff check echo directly to $GITHUB_OUTPUT and avoid the $GITHUB_ENV middle man.

@ameba23
Copy link
Collaborator Author

ameba23 commented May 2, 2025

Thats great - using this correctly triggered the tests to run. Yeah i see now that that code was pretty stupid. But whats strange is that it's been like that for several weeks and no-one including myself has complained about it until now.

Thanks so much @cooldracula

@ameba23 ameba23 merged commit e1a017c into master May 2, 2025
9 checks passed
@ameba23 ameba23 deleted the peg/fix-code-changed-check branch May 2, 2025 06:17
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.

2 participants