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

Shuffle tests for a single worker #17149

Closed
wants to merge 1 commit into from

Conversation

dhuang00
Copy link
Contributor

@dhuang00 dhuang00 commented Dec 13, 2024

Hi,

I'm using run-tests.php for testing in extension development. For my use case, I require the tests to be run one at a time and therefore cannot make use of the -j<workers> flag.

After a bit of investigation, I found that the --shuffle flag is hidden behind parallelization.

By moving shuffling to happen earlier in run_all_tests rather than run_all_tests_parallel, I was able to find some issues with my tests being order dependent.

Would this contribution be something of interest to the community?

Even when tests are not run in parallel, shuffling can help discover tests that
unintentionally depend on other tests being run before them.
@dhuang00 dhuang00 marked this pull request as ready for review December 13, 2024 20:52
@Girgias
Copy link
Member

Girgias commented Dec 13, 2024

I think the change makes sense overall. But I know for sure that the tests of ext/pgsql are order dependent.

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

LGTM.

But I know for sure that the tests of ext/pgsql are order dependent.

This would already be an issue, right? It's not a new flag.

@Girgias
Copy link
Member

Girgias commented Dec 17, 2024

LGTM.

But I know for sure that the tests of ext/pgsql are order dependent.

This would already be an issue, right? It's not a new flag.

Right, but this is "prevented" by the use of the ext/pgsql/tests/CONFLICTS file to mark all tests in this folder as conflicting, which currently ensures they are run in order.

@cmb69
Copy link
Member

cmb69 commented Dec 18, 2024

But I know for sure that the tests of ext/pgsql are order dependent.

That should be fixed. :)

@Girgias
Copy link
Member

Girgias commented Dec 18, 2024

But I know for sure that the tests of ext/pgsql are order dependent.

That should be fixed. :)

I do agree, but how it is actually setup currently, makes sense.

@nielsdos nielsdos closed this in 71dfa93 Dec 27, 2024
@nielsdos
Copy link
Member

Looks like this was not merged yet but had an approval. I've merged this into master along with a NEWS entry.
Thanks for your contribution!

@cmb69
Copy link
Member

cmb69 commented Dec 27, 2024

@nielsdos, that patch may cause issues with ext/pgsql tests, which are order dependent according to @Girgias.

@nielsdos
Copy link
Member

@cmb69 I interpreted "That should be fixed." as that it was already fixed.
Anyway, looking at the test code I don't think this can cause issues right now? If you ran tests in parallel with shuffle on, you would've experienced the issue already. And if you ran it in a single process then you couldn't shuffle.

@Girgias
Copy link
Member

Girgias commented Dec 27, 2024

@cmb69 I interpreted "That should be fixed." as that it was already fixed. Anyway, looking at the test code I don't think this can cause issues right now? If you ran tests in parallel with shuffle on, you would've experienced the issue already. And if you ran it in a single process then you couldn't shuffle.

See one of my prior comments :)

But I know for sure that the tests of ext/pgsql are order dependent.

This would already be an issue, right? It's not a new flag.

Right, but this is "prevented" by the use of the ext/pgsql/tests/CONFLICTS file to mark all tests in this folder as conflicting, which currently ensures they are run in order.

Basically CONFLICTS says that all the tests in this folder conflict, and thus are never run in parallel.
I don't think it's a fundamental blocker if we add a way to prevent the tests from being shuffled (maybe adding a file ORDER_DEPENDENT ?). Would also make it easy to see which sets of tests are order dependent.

@nielsdos
Copy link
Member

Ah okay, I can see this logic in the test runner indeed that it puts the conflicts at the end on a single worker.
It's probably going to be better to fix the test ordering instead of trying to patch it up though.

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.

5 participants