search

SERG-Delft / andy

Andy assesses student's test code. It's used in CSE1110, TU Delft.
MIT License
78 stars 22 forks source link

Improved performance of the github workflow #236

Closed alexcojocaru2002 closed 1 year ago

alexcojocaru2002 commented 1 year ago

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 commented 1 year ago

We gotta review this one together with #238

Arraying commented 1 year ago

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

mauricioaniche commented 1 year ago

@martinmladenov can you work on this one?

mauricioaniche commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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

alexcojocaru2002 commented 1 year ago

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 commented 1 year ago

All tests should be passing! Maybe rebase with master ?

alexcojocaru2002 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Pipeline running!

alexcojocaru2002 commented 1 year ago

Pipeline running!

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

mauricioaniche commented 1 year ago

We saw this error before in this PR. This doesn't happen in master. Can you double check your code is rebased?

alexcojocaru2002 commented 1 year ago

We saw this error before in this PR. This doesn't happen in master. Can you double check your code is rebased?

Rebased again just in case. I also checked the code and it looks synced with the main branch here.

mauricioaniche commented 1 year ago

@martinmladenov im guessing it's the maven cache here.

@alexcojocaru2002 can you add a mvn install to the script? If you look at the history of the file, you will find it. I think we removed it quite recently, but my feeling is that it's needed!

alexcojocaru2002 commented 1 year ago

We saw this error before in this PR. This doesn't happen in master. Can you double check your code is rebased?

They probably work because the jobs include the 'Compile and Install everything' step that I deleted. I thought this step was unnecessary but it seems necessary at least for weblab tests.

alexcojocaru2002 commented 1 year ago

Yep, I will, and it should be done 👍

mauricioaniche commented 1 year ago

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

alexcojocaru2002 commented 1 year ago

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 commented 1 year ago

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

martinmladenov commented 1 year ago

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

martinmladenov commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

martinmladenov commented 1 year ago

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

mauricioaniche commented 1 year ago

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

martinmladenov commented 1 year ago

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

martinmladenov commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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