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

Consolidating duplicate excluded tests #4651

Merged

Conversation

adamfarley
Copy link
Contributor

@adamfarley adamfarley commented Jun 29, 2023

Commenting out duplicate tests from the exclude files, ensuring the platforms are appended to their unexcluded duplicates, and tidying the targets to help avoid this problem in the future.

See here for details:

#4650

@adamfarley
Copy link
Contributor Author

adamfarley commented Jun 29, 2023

Duplicates have been removed. Tidying now.

@karianna karianna marked this pull request as draft June 30, 2023 03:03
Duplicates can cause tests to be unexcluded, so I've removed all
instances of this by commenting out the duplicates and
consolidating the platforms in the non-commented-out exclusion.

Plus tidying. Whitespace management, some new lines for uniformity,
etc.

Signed-off-by: Adam Farley <adfarley@redhat.com>
@adamfarley adamfarley force-pushed the consolidating_duplicate_excluded_tests branch from 0d107b8 to 727881b Compare July 5, 2023 10:52
@adamfarley adamfarley changed the title WIP: Consolidating duplicate excluded tests Consolidating duplicate excluded tests Jul 5, 2023
@adamfarley adamfarley marked this pull request as ready for review July 5, 2023 10:53
@karianna
Copy link
Contributor

karianna commented Jul 6, 2023

@adamfarley Lots of failing test?

@adamfarley adamfarley closed this Jul 6, 2023
@adamfarley adamfarley reopened this Jul 6, 2023
@smlambert
Copy link
Contributor

We need to update our workflows (captured in #2299)

  • to not run all the tests when these files change
  • to ensure Github runners are configured with up-to-date prereqs

Will not block this PR on those failures. (Jenkins Grinder run as a better test for this PR than using run-aqa in a workflow).

@smlambert smlambert merged commit 61f9804 into adoptium:master Jul 6, 2023
@adamfarley
Copy link
Contributor Author

adamfarley commented Jul 6, 2023

@adamfarley Lots of failing test?

Ok, got a theory vis-a-vis the failures:

Looking at a bunch of other PRs in this repo (including this one, which is also an exclude file change), it seems that these tests aren't run for any other PRs.

My theory is that, since this PR started out as a WIP PR, it triggered the test jobs (via the github action "Directories/Files Change Test PR") when I took the WIP out of the title and clicked the "ready for review" button.

So, in short, I think we should merge this PR as-is, and raise 2 separate issues for the following problems (to be addressed AFTER the release ofc):

  1. That the Git action for these tests appears to have been designed to run on every PR, and is instead only run when someone changes a PR from draft to ready-for-review. If we are meant to be running these tests for every PR, this needs fixing.
  2. That there are a number of issues in these tests which need to be solved. Some issues look familiar, so perhaps they're all already solved and the fixes just need to be integrated into the Git action.

Update: Shelley beat me to it. :)

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.

4 participants