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

CI workflow to perform automated testing with multithreading #260

Merged

Conversation

joshichaitanya3
Copy link
Collaborator

This PR attempts to add a CI workflow to run the test-suite with morpho6 -w2, thus checking for bugs with multithreading.

Given the increase in parallelization in Morpho (yay!), this could be quite useful to spot bugs like #259

It essentially adds one more argument flag to test.py and invokes it in the new workflow.

Copy link
Contributor

@softmattertheory softmattertheory left a comment

Choose a reason for hiding this comment

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

This looks good. Suggested changes:

  1. Change branches on lines 5, 7 to include "dev"

  2. test.py lines 186 - 193 we can do something a bit more elegant, e.g.

for a in sys.argv:
    if a == '-c': 
        CI = True 
    elif a == '-w':
        # do something else 
  1. line 198 append "-w4" to command:
command+="-w4" 

More threads are better to make a race condition more likely to appear.

@joshichaitanya3
Copy link
Collaborator Author

Great, thanks! I have fixed these.

  1. Change branches on lines 5, 7 to include “dev"

I had copied this workflow from the buildandtest one, and it didn’t have dev too. Is that something we need to fix?

@softmattertheory
Copy link
Contributor

I had copied this workflow from the buildandtest one, and it didn’t have dev too. Is that something we need to fix?

Good point: I already fixed it, but the fix is not in dev, it's in type. [See, e.g. https://github.com/Morpho-lang/morpho/blob/type/.github/workflows/nonanboxing.yml]

@softmattertheory
Copy link
Contributor

Suggestion, @joshichaitanya3: Could you change the target of this PR to the morehessians branch? I've now fixed the race conditions—this was great we discovered them!—and so this test should work for that branch.

@joshichaitanya3 joshichaitanya3 changed the base branch from dev to morehessians May 22, 2024 13:13
@joshichaitanya3
Copy link
Collaborator Author

Sure thing, I just did!

@softmattertheory softmattertheory merged commit 57421be into Morpho-lang:morehessians May 22, 2024
1 check failed
@joshichaitanya3 joshichaitanya3 deleted the mutithreading-ci branch May 23, 2024 16:01
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