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

Improved performance of the github workflow #236

Merged
merged 25 commits into from
Jul 20, 2023

Conversation

alexcojocaru2002
Copy link
Contributor

@alexcojocaru2002 alexcojocaru2002 commented Jul 8, 2023

Closes #214

In this merge request only one random test is run on docker from each category (seen in the python script) and the workflow has been split in one job for each category to improve performance.

image

@mauricioaniche
Copy link
Contributor

We gotta review this one together with #238

@Arraying
Copy link
Contributor

Arraying commented Jul 9, 2023

@mauricioaniche I'm happy for you to just review and this one, and then I'll incorporate the changes in my branch.

@mauricioaniche
Copy link
Contributor

@martinmladenov can you work on this one?

Copy link
Contributor

@mauricioaniche mauricioaniche 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 to me!!

@martinmladenov any thoughts? If not, feel free to merge it!

@mauricioaniche
Copy link
Contributor

@alexcojocaru2002 There's a small conflict now, as we've added checkstyle to the build. I think it should run before the tests, and if it breaks, we can fail fast the build. Can you make this change?

@alexcojocaru2002
Copy link
Contributor Author

@alexcojocaru2002 There's a small conflict now, as we've added checkstyle to the build. I think it should run before the tests, and if it breaks, we can fail fast the build. Can you make this change?

Sorry for the late reply, I am in vacation for a bit as well 😅 I will update the changes based on the checkstyle today or tomorrow 👍 If there are any other things to add please let me know

@martinmladenov
Copy link
Collaborator

What's the difference between run_unit_tests and run_andy_tests? Those both seem to do the same thing.

@alexcojocaru2002
Copy link
Contributor Author

Also @martinmladenov @mauricioaniche has there been any changes to the weblab tests ? They seem to fail now due to a missing dependency, I will see if I can fix it but maybe something changed in the last PRs that I should be aware of and I was not able to check😅

@mauricioaniche
Copy link
Contributor

All tests should be passing! Maybe rebase with master ?

@alexcojocaru2002
Copy link
Contributor Author

alexcojocaru2002 commented Jul 19, 2023

All tests should be passing! Maybe rebase with master ?

I fixed it locally by doing a clean install before the weblab tests, I am not sure why... I will try to see if they pass without it first. I already had a rebase with master from Martin

@alexcojocaru2002
Copy link
Contributor Author

alexcojocaru2002 commented Jul 19, 2023

All tests should be passing! Maybe rebase with master ?

Can I get a quick approval on the workflows to test if they work here ?

I think if they do not work adding the clean install step that I deleted earlier only for the weblab tests would be a not-so-bad alternative. Even with that step, the runtime of the workflow is similar (because the assignment tests take longer than weblab tests + clean reinstall).

Ideally, a check on why this is required only for the weblab tests would be best but as a quick fix the above works. The only thing I see is that the dependencies cant be found anymore

@mauricioaniche
Copy link
Contributor

Pipeline running!

@alexcojocaru2002
Copy link
Contributor Author

alexcojocaru2002 commented Jul 19, 2023

Pipeline running!

The pipeline gives some errors regarding the ContextDirector / Builder in the AndyOnWeblab class, interesting 🤔 .

@alexcojocaru2002
Copy link
Contributor Author

For the weblab module, the Andy jar needs to be available. So, my guess would be that before running weblab tests, we need to do a mvn install -pl andy

I did the original clean install -Dskiptests for shorter time, it seems to work now 👍. Sorry for the large number of commits, I had to test different small changes

@mauricioaniche
Copy link
Contributor

Great to hear it works! @martinmladenov can you give it a final look and merge ?

.github/workflows/tests.yml Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
@martinmladenov
Copy link
Collaborator

I just changed the and now the pipeline fails. Seems like it can no longer resolve dependencies for some reason?

@martinmladenov
Copy link
Collaborator

Is there a way to reduce code duplication in the workflow configuration file? I see that every task has the steps "Checkout the repository", "Set up JDK 17" and "Cache Maven packages". Is there a way to define them only once in the file and make them apply to all tasks?

@martinmladenov
Copy link
Collaborator

One of the steps in the Docker task is called "Run the reference solutions of all assignments and verify the scores are 100/100". This is no longer correct, it should say "some assignments" or something similar.

@mauricioaniche
Copy link
Contributor

I just changed the and now the pipeline fails. Seems like it can no longer resolve dependencies for some reason?

Everything seems green here! Shall we merge this one?

@martinmladenov
Copy link
Collaborator

It seems to pass now. Not sure what happened there. We can merge, but I left some more comments: see above.

@martinmladenov
Copy link
Collaborator

I'll turn those comments into discussions so that they show up more clearly, this PR is very long now.

@mauricioaniche
Copy link
Contributor

We can reduce duplication in another MR to! I'd say we merge this one as soon as pipeline passes!

@martinmladenov
Copy link
Collaborator

martinmladenov commented Jul 20, 2023

We also need to change the repository configuration before we can merge this one. I'll do it now.

@martinmladenov
Copy link
Collaborator

@mauricioaniche That is so weird. Jobs seem to fail in the fork but pass on our repo https://github.com/alexcojocaru2002/andy/actions/runs/5615314967/job/15215354923

@alexcojocaru2002
Copy link
Contributor Author

Is there a way to reduce code duplication in the workflow configuration file? I see that every task has the steps "Checkout the repository", "Set up JDK 17" and "Cache Maven packages". Is there a way to define them only once in the file and make them apply to all tasks?

Unfortunately github workflow is a bit behind compared to its competitors when it comes to code duplication. There is not option to use yaml anchors which would ve made this way shorter in terms of code duplication. I thought of the file option but there is no way to take the environment of a job and continue it that i know of.

@alexcojocaru2002
Copy link
Contributor Author

We can reduce duplication in another MR to! I'd say we merge this one as soon as pipeline passes!

Can be done in the one that should fix windows for pipeline, maybe, since it s going to add even more duplicated code

@martinmladenov
Copy link
Collaborator

I see, thanks for the explanation and for your help with improving our CI! :)

@mauricioaniche mauricioaniche merged commit c856ece into SERG-Delft:main Jul 20, 2023
8 checks passed
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.

Make assignment tests faster
4 participants