[#1630] Rearrange Gradle tasks and their dependencies #1631

yhtMinceraft1010X · 2022-01-25T15:24:40Z

Proposed commit message:

When running pure backend tests alone using gradlew test systemtest, the
frontend is built before these tests. This is due to zipReport task
being needed for compileJava. zipReport in turn relies on buildFrontend.

The frontend is not used for these tests. It is wasteful to run 
buildFrontend when only the backend tests are being run.

In addition, some other tasks and properties are scattered throughout
the build.gradle file or have dependencies that could be pushed forward.
This complicates maintenance.

Let's remove the `test` and `systemtest` dependency on the frontend as
well as re-order and consolidate some task properties and dependencies.
This should make running either of them faster.

gerhean · 2022-01-26T02:53:01Z

Is it possible to just remove zipReport since it is just zipping an empty folder if buildFrontend is not run? Or is the real problem that buildFrontend needs to be run at least once, but the current script runs it multiple times which is redundant (Which means your code will cause unexpected bugs by not requiring buildFrontend)?

Actually the entire process is a little weird and should probably be overhauled. Maybe introduce a build everything command, and maybe add checks that the required components exist for each command else build them.

For example, startServerInBackground has some weird dependency issues.

dcshzj · 2022-01-26T04:22:30Z

Actually the entire process is a little weird and should probably be overhauled. Maybe introduce a build everything command, and maybe add checks that the required components exist for each command else build them.

I think let's discuss this in #1630 to see what solutions we should adopt. Let's not rush into implementation yet. @gerhean Do you mind copying your comment over and to continue the discussion on #1630?

yhtMinceraft1010X · 2022-03-13T17:09:45Z

Now that #1691 has been merged, I'm re-opening this pull request.

The zipReport task is still needed for putting the frontend assets in src/main/resources. This is especially important for generating a jar file for releases such that the frontend assets don't need to be downloaded separately.

However, pure backend tests and systemtests do not need any of the frontend assets.

gok99 · 2022-03-13T19:12:22Z

build.gradle

-tasks.run.dependsOn('compileJava');
-
-task systemtest(dependsOn: 'zipReport', type: Test) {
+task systemtest(type: Test) {


This might be a separate discussion but our systemtests currently are more of an integrated pure backend test. In addition to bypassing CLI arguments (as is an issue raised in #1645), we also don't seem to have any tests for the integration of front and backend - specifically the correct replication of template.zip files.

Should we also be doing frontend-backend integration tests in systemtests as well?

specifically the correct replication of template.zip files.

I think this can be settled by changing ConfigSystemTest to compare the index.html file since right now it only compares the .json files. Although this would require someone to 'validate' the html file each time something is changed that changes the expected html

For any changes to ConfigSystemTest, I think they are best done in a separate PR. However, if index.html is going to be compared, the dependency on zipReport should be added back in that PR

I think they are best done in a separate PR

Yes, I agree. I think it's also okay to leave it as-is and add back the dependency if we decide to check frontend build files.

I just want to say that I prefer for zipReport to be removed completely since it is basically a convoluted way of copying files around, instead of just copying it directly to the output folder.
In my opinion the Java code should not be responsible for copying around the files. That is what gradle is supposed to be for.
Well unless it is supposed to be used as a Jar file?

In any case the index.html file is more or less constant, I'm not sure if there is really a need to check if it is copied correctly if no changes are done to it.

From what I know, zipReport would still be needed to ensure that there is a templateZip.zip file in src/main/resources so that all the frontend assets in the zip file can be packaged in the jar file when running shadowJar.

Once a release jar file is downloaded by someone, the backend code is needed to copy the frontend assets inside to the output directory. This enables viewing the report in browser. Gradle is only relevant for users who fork the entire project.

build.gradle

gok99 · 2022-03-13T19:17:04Z

.github/workflows/integration.yml

@@ -67,7 +67,7 @@ jobs:
      run: ./gradlew lintFrontend

    - name: Build with Gradle
-      run: time ./gradlew clean checkstyleMain checkstyleTest test systemTest coverage
+      run: time ./gradlew clean checkstyleMain checkstyleTest zipReport test systemTest coverage


Would not including zipReport actually affect anything? If it's for the later artifact upload perhaps we should create a separate gradle task that explicitly depends on zipReport for this purpose to make it clear that the test is using a zipReport (since the behavior of the tests with and without zipReport is now different)

I put this here because I thought Build preview website may need it. Initially, zipReport would have been done as compileJava was entirely dependent on it. But in this case, with the general dependency removed, none of the tasks here would have called zipReport.

Yea, it seems like a separate gradle run is done for Build preview website. Perhaps @dcshzj could confirm that there is no dependency here between the steps.

The executionData for jacocoTestReport are at the end of build.gradle while the jacocoTestReport is configured early on in the file. This hinders any future edits to jacocoTestReport's properties. Let's consolidate executionData inside jacocoTestReport and move jacoco and jacocoTestReport to allow for systemtest and frontendTest to be detected correctly.

yhtMinceraft1010X · 2022-03-30T02:22:43Z

Closing and reopening due to Expected - Waiting for status to be reported.

github-actions · 2022-03-30T02:22:56Z

The following links are for previewing this pull request:

Dashboard Preview: https://dashboard-1631-pr-reposense-reposense.surge.sh
Docs Preview: https://docs-1631-pr-reposense-reposense.surge.sh

chan-j-d

LGTM! Correct me if my understanding is wrong, but most of the line changes here are reordering to group related tasks together? The key line changes here is removing the need to run zipReport in systemTest and compileJava which now allows us to run tests and system tests without building frontend.

I've tested the various tasks and it seems to work great. Thanks for doing this.

yhtMinceraft1010X · 2022-04-05T06:47:08Z

My original intention was to remove the need to run zipReport for pure backend tests but I also realised that there were also some other areas where refactoring could be done, such as putting zipReport as a dependency for serveTestReportInBackground since it is needed for the 2 cypress-related tasks anyway.

Another point of note is that for build.gradle, some tasks can only be referenced if declared before hand. I moved the Jacoco tasks to the back when consolidating the executionData since the build would fail as systemtest and frontendTest were not yet defined when jacocoTestReport was in front of both tests.

yhtMinceraft1010X · 2022-04-05T06:50:07Z

Correct me if my understanding is wrong, but most of the line changes here are reordering to group related tasks together? The key line changes here is removing the need to run zipReport in systemTest and compileJava which now allows us to run tests and system tests without building frontend.

Now that you've mentioned it, I've edited the commit message and PR title to reflect the new changes.

chan-j-d · 2022-04-05T07:38:33Z

I see! thanks for the explanation.

dcshzj

LGTM! Looks like the CI tests run a little faster now too

github-actions · 2022-04-11T02:24:06Z

The following links are for previewing this pull request:

Dashboard Preview: https://dashboard-1631-pr-reposense-reposense.surge.sh
Docs Preview: https://docs-1631-pr-reposense-reposense.surge.sh

yhtMinceraft1010X added 7 commits August 27, 2021 07:51

Merge branch 'master' of https://github.com/reposense/RepoSense

ab60526

Merge branch 'master' of https://github.com/reposense/RepoSense

1f3cb05

Merge branch 'master' of https://github.com/reposense/RepoSense

4142873

Merge branch 'master' of https://github.com/reposense/RepoSense

6ba4885

Merge branch 'master' of https://github.com/reposense/RepoSense

5403b80

Remove frontend dependency for backend tests

0e488bd

Fix cypress not running automatically

037a561

yhtMinceraft1010X requested a review from a team January 25, 2022 15:24

Fix NullPointerException for zipReport

9a3121c

yhtMinceraft1010X marked this pull request as draft January 25, 2022 15:40

Fix systemtests

d7542fd

yhtMinceraft1010X marked this pull request as ready for review January 26, 2022 02:41

dcshzj closed this Jan 26, 2022

gerhean mentioned this pull request Jan 26, 2022

Remove building of frontend when running backend tests #1630

Closed

yhtMinceraft1010X added 3 commits February 19, 2022 22:01

Merge branch 'master' into 1630-improve-gradle

7d5c248

Merge branch 'master' into 1630-improve-gradle

24aa665

Merge branch 'master' into 1630-improve-gradle

ea2927e

yhtMinceraft1010X mentioned this pull request Feb 28, 2022

[#1659] Separate backend and frontend responsibilites #1691

Merged

yhtMinceraft1010X added 3 commits March 1, 2022 14:09

Merge branch 'master' into 1630-improve-gradle

3b46dae

Merge branch 'master' into 1630-improve-gradle

49f6e56

Update task dependencies

d9d9abb

yhtMinceraft1010X reopened this Mar 13, 2022

yhtMinceraft1010X requested a review from a team March 13, 2022 17:13

gok99 reviewed Mar 13, 2022

View reviewed changes

yhtMinceraft1010X added 2 commits March 14, 2022 13:41

Rename startServerInBackground

c75a416

Remove zipReport from CI

815d232

yhtMinceraft1010X mentioned this pull request Mar 14, 2022

Upgrade Gradle to 6.9.2 #1664

Merged

dcshzj added the s.ToReview label Mar 18, 2022

yhtMinceraft1010X added 4 commits March 23, 2022 18:50

Merge branch 'master' into 1630-improve-gradle

40ba0d5

Merge branch 'master' into 1630-improve-gradle

06c0dc9

Merge branch 'master' into 1630-improve-gradle

732d0b6

yhtMinceraft1010X closed this Mar 30, 2022

yhtMinceraft1010X reopened this Mar 30, 2022

Merge branch 'master' into 1630-improve-gradle

ef6687b

chan-j-d approved these changes Apr 4, 2022

View reviewed changes

yhtMinceraft1010X changed the title ~~[#1630] Remove building of frontend when running backend tests~~ [#1630] Reorder Gradle Tasks and Dependencies Apr 5, 2022

dcshzj approved these changes Apr 9, 2022

View reviewed changes

dcshzj added s.ToMerge and removed s.ToReview labels Apr 9, 2022

Merge branch 'master' into 1630-improve-gradle

905d3f1

dcshzj changed the title ~~[#1630] Reorder Gradle Tasks and Dependencies~~ [#1630] Rearrange Gradle tasks and their dependencies Apr 11, 2022

dcshzj merged commit fe4271f into reposense:master Apr 11, 2022

yhtMinceraft1010X deleted the 1630-improve-gradle branch April 15, 2022 14:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1630] Rearrange Gradle tasks and their dependencies #1631

[#1630] Rearrange Gradle tasks and their dependencies #1631

yhtMinceraft1010X commented Jan 25, 2022 •

edited by dcshzj

Loading

gerhean commented Jan 26, 2022 •

edited

Loading

dcshzj commented Jan 26, 2022

yhtMinceraft1010X commented Mar 13, 2022

gok99 Mar 13, 2022 •

edited

Loading

chan-j-d Mar 14, 2022 •

edited

Loading

yhtMinceraft1010X Mar 14, 2022

gok99 Mar 14, 2022

gerhean Mar 15, 2022 •

edited

Loading

yhtMinceraft1010X Mar 17, 2022

gok99 Mar 13, 2022 •

edited

Loading

yhtMinceraft1010X Mar 14, 2022

gok99 Mar 14, 2022

yhtMinceraft1010X commented Mar 30, 2022

github-actions bot commented Mar 30, 2022

chan-j-d left a comment

yhtMinceraft1010X commented Apr 5, 2022

yhtMinceraft1010X commented Apr 5, 2022

chan-j-d commented Apr 5, 2022

dcshzj left a comment

github-actions bot commented Apr 11, 2022

[#1630] Rearrange Gradle tasks and their dependencies #1631

[#1630] Rearrange Gradle tasks and their dependencies #1631

Conversation

yhtMinceraft1010X commented Jan 25, 2022 • edited by dcshzj Loading

gerhean commented Jan 26, 2022 • edited Loading

dcshzj commented Jan 26, 2022

yhtMinceraft1010X commented Mar 13, 2022

gok99 Mar 13, 2022 • edited Loading

Choose a reason for hiding this comment

chan-j-d Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

yhtMinceraft1010X Mar 14, 2022

Choose a reason for hiding this comment

gok99 Mar 14, 2022

Choose a reason for hiding this comment

gerhean Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

yhtMinceraft1010X Mar 17, 2022

Choose a reason for hiding this comment

gok99 Mar 13, 2022 • edited Loading

Choose a reason for hiding this comment

yhtMinceraft1010X Mar 14, 2022

Choose a reason for hiding this comment

gok99 Mar 14, 2022

Choose a reason for hiding this comment

yhtMinceraft1010X commented Mar 30, 2022

github-actions bot commented Mar 30, 2022

chan-j-d left a comment

Choose a reason for hiding this comment

yhtMinceraft1010X commented Apr 5, 2022

yhtMinceraft1010X commented Apr 5, 2022

chan-j-d commented Apr 5, 2022

dcshzj left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 11, 2022

yhtMinceraft1010X commented Jan 25, 2022 •

edited by dcshzj

Loading

gerhean commented Jan 26, 2022 •

edited

Loading

gok99 Mar 13, 2022 •

edited

Loading

chan-j-d Mar 14, 2022 •

edited

Loading

gerhean Mar 15, 2022 •

edited

Loading

gok99 Mar 13, 2022 •

edited

Loading