Skip to content

Test suite clean up #3385

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 97 commits into from
Jan 23, 2025
Merged

Test suite clean up #3385

merged 97 commits into from
Jan 23, 2025

Conversation

JDBetteridge
Copy link
Member

@JDBetteridge JDBetteridge commented Feb 2, 2024 •
edited by connorjward
Loading

Description

  • Switches to having MPI executed 'on the outside'. This means we are no longer bound to MPICH (the source of huge build times and MacOS bugs) and parallel tests do not oversubscribe the runners.
  • Includes various associated bug fixes since parallel tests are not run in isolation.

TODOs

  • Fix remaining failing tests
  • Add make check and clean up Makefile (and use in CI + mention on install page)
  • Remove --download-mpich from everywhere, means that MPI is a system dependency (do as part of switch to pip)

@connorjward
Copy link
Contributor

connorjward commented Feb 2, 2024 •
edited
Loading

This is cool, but isn't it a bad idea to effectively remove test coverage? If CI doesn't run all the tests no one will.

I can see this being useful in the context of a bigger change where we run the test suite with a number of Firedrake configurations and only one of them would run these slow tests.

@JDBetteridge JDBetteridge force-pushed the JDBetteridge/faster_tests branch from ecb9628 to f3685b7 Compare February 9, 2024 19:21
@JDBetteridge JDBetteridge force-pushed the JDBetteridge/faster_tests branch from f3685b7 to f848c6f Compare March 5, 2024 13:40
@JDBetteridge JDBetteridge force-pushed the JDBetteridge/faster_tests branch 2 times, most recently from 93a0aae to a20fc9d Compare June 7, 2024 13:52
@JDBetteridge JDBetteridge force-pushed the JDBetteridge/faster_tests branch from a5f614f to 27b9af3 Compare July 19, 2024 16:25
@JDBetteridge JDBetteridge force-pushed the JDBetteridge/faster_tests branch from 2a7f468 to 6ed1774 Compare August 18, 2024 13:44
@JDBetteridge JDBetteridge force-pushed the JDBetteridge/faster_tests branch 2 times, most recently from 8132b64 to 1122a3b Compare August 31, 2024 18:37
@JDBetteridge JDBetteridge self-assigned this Sep 4, 2024
JHopeCollins
JHopeCollins previously requested changes Sep 10, 2024
View reviewed changes
JDBetteridge
JDBetteridge commented Sep 11, 2024
View reviewed changes
@JDBetteridge JDBetteridge force-pushed the JDBetteridge/faster_tests branch 2 times, most recently from 9d5f056 to df4aea3 Compare September 12, 2024 13:04
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 12, 2024 •
edited
Loading

TestsPassed ✅Skipped ⏭️Failed ❌
Firedrake complex8125 ran6557 passed1568 skipped0 failed

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 12, 2024 •
edited
Loading

TestsPassed ✅Skipped ⏭️Failed ❌
Firedrake real8116 ran7400 passed716 skipped0 failed

@JDBetteridge JDBetteridge marked this pull request as ready for review September 24, 2024 14:23
@JDBetteridge JDBetteridge force-pushed the JDBetteridge/faster_tests branch from bf317f5 to ef87021 Compare October 2, 2024 21:57
@connorjward connorjward changed the title Mark and skip slow tests Test suite clean up Oct 3, 2024
connorjward
connorjward reviewed Oct 3, 2024
View reviewed changes
Copy link
Contributor

@connorjward connorjward left a comment

Choose a reason for hiding this comment

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

Generally very happy with this.

@JDBetteridge JDBetteridge force-pushed the JDBetteridge/faster_tests branch from 8435a69 to 10f7f0c Compare October 8, 2024 14:02
@connorjward connorjward mentioned this pull request Jan 21, 2025
Merged
connorjward
connorjward reviewed Jan 22, 2025
View reviewed changes
connorjward
connorjward reviewed Jan 22, 2025
View reviewed changes
JHopeCollins
JHopeCollins requested changes Jan 22, 2025
View reviewed changes
Copy link
Member

@JHopeCollins JHopeCollins left a comment

Choose a reason for hiding this comment

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

I've just looked over the setup for running tests in parallel. I think it's shaping up well, it'll be nice if we can just rely on standard pytest and bash components for this.

I've have a few comments, but none of them are major.

connorjward and others added 3 commits January 22, 2025 15:32
@connorjward @JHopeCollins
Apply suggestions from code review
df0d9ca 
Co-authored-by: Josh Hope-Collins <joshua.hope-collins13@imperial.ac.uk>
@connorjward
Address code review comments
113a6bb
@connorjward
last bits
c880af8
JHopeCollins
JHopeCollins requested changes Jan 22, 2025
View reviewed changes
dham
dham previously requested changes Jan 22, 2025
View reviewed changes
@JHopeCollins JHopeCollins dismissed dham’s stale review January 23, 2025 12:37

Changes applied.

@JHopeCollins JHopeCollins self-requested a review January 23, 2025 12:37
JHopeCollins
JHopeCollins approved these changes Jan 23, 2025
View reviewed changes
Copy link
Member

@JHopeCollins JHopeCollins left a comment

Choose a reason for hiding this comment

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

This is great, thanks @connorjward and @JDBetteridge for all your work on this!

@JHopeCollins JHopeCollins enabled auto-merge (squash) January 23, 2025 12:44
@connorjward connorjward disabled auto-merge January 23, 2025 13:13
@connorjward connorjward merged commit ce571da into master Jan 23, 2025
20 checks passed
@connorjward connorjward deleted the JDBetteridge/faster_tests branch January 23, 2025 13:15
indiamai pushed a commit that referenced this pull request Feb 27, 2025
@JDBetteridge @indiamai
Test suite clean up (#3385)
834db29 
* Split apart test suite into those run on different numbers of ranks. Testing is now using MPI 'on the outside' which should allow us to stop relying on MPICH.
* Various fixes for parallel tests now they are not run in isolation.
* Added `firedrake-run-split-tests` script which other packages may wish to use.
indiamai pushed a commit that referenced this pull request Apr 29, 2025
@JDBetteridge @indiamai
Test suite clean up (#3385)
9197716 
* Split apart test suite into those run on different numbers of ranks. Testing is now using MPI 'on the outside' which should allow us to stop relying on MPICH.
* Various fixes for parallel tests now they are not run in isolation.
* Added `firedrake-run-split-tests` script which other packages may wish to use.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@connorjward connorjward connorjward left review comments

@JHopeCollins JHopeCollins JHopeCollins approved these changes

@dham dham dham left review comments

Assignees

@connorjward connorjward

Labels
bug enhancement macOS performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

INSTALL: Tests not passing on fresh install M2 Mac
5 participants
@JDBetteridge @connorjward @dham @JHopeCollins @Ig-dolci