serenity-bdd / serenity-gradle-plugin

Apache License 2.0
1 stars 11 forks source link

gradle discrepancies #33

Open zeners opened 5 months ago

zeners commented 5 months ago

i found the following needed actions, please correct me, if i'm wrong

i tested the actions with a simple test-project and with active --build-cache --configuration-cache getting the following result:

gradle writes output into build, serenity into target/site/serenity, clean only remove serenity inside target/site

petrandreev commented 5 months ago

hi @zeners, regarding your Q about generateOutcomes, not needed before- it's been changed here:https://github.com/serenity-bdd/serenity-gradle-plugin/commit/3424830cb766d87cc669524396988cb8dc9dd444 It used to be always set, now one needs to set it to "true" on PluginExtension explicitely. Thanks for summing up

zeners commented 5 months ago

i can't test the deployment, so i hope that i didn't broke something

wakaleo commented 5 months ago

Just published the new version, feel free to test

zeners commented 5 months ago

@wakaleo thank you

the cache doesn't work as expected. I'm afraid that we have to use the complete outputDirectory from test.

tnielens commented 5 months ago

Hi here, Sorry to jump in. Was #19 reverted? From what I tested only #30 to fix the missing test results needed to be merged for it to work properly. Internally we're using a published version including #30 and we observe no issues with the plugin.

zeners commented 5 months ago

c7e03104d352446d0b4b8a80094075b3c762b6c7 was an undo of #19, but 3424830cb766d87cc669524396988cb8dc9dd444 makes a redo, but with differences

currently i'm afraid that's not possible to make a consistent result, if you test without aggegate (test -x aggregate) and after it clean test will run aggregate without test-results, not restored from cache.

i have a plan in mind, but need some time to implement and test

tnielens commented 5 months ago

Thanks for clarifying.

As for aggregate, we don't understand well its behavior as it doesn't play well with the project-scoped expectation of gradle. That task seems to collect outputs from all subprojects in a multimodule project although the task exists in each project. Also the static/global state of serenity prevents use of gradle parallelism and running parallel tasks in a multi-module project leads to concurrency issues in serenity tasks and outputs.

Aside of these issues, #19 was a welcome improvement 👍.

zeners commented 5 months ago

That task seems to collect outputs from all subprojects in a multimodule project although the task exists in each project.

assuming the gradle-plugin should work like the maven-plugin, as outlined in https://github.com/serenity-bdd/serenity-maven-plugin/issues/22#issuecomment-176633201 you have to set a common output-directory in all subprojects, to get one report for all subproject tests, correct?

petrandreev commented 5 months ago

Solved reports generation issue in a muli-module project with v 4.1.10 (happens on RH UBI, e.g. linux, not on Mac though...) on my side. IMHO the issue has nothing to do with gradle cache(s) -

  1. test task (JSONTestOutcomeReporter actually) generates reports into serenity.conf's outputDirectory (build/site/serenity)
  2. on aggregate task, Gradle decides that the whole directory, configured by serenity plugin extension's outputDirectory is stale and deletes it!:
    > Task :serenity:aggregate
    Deleting stale output file: ......../build/site/serenity
  3. aggregator has nothing to work with since no JSON outcomes - no reports in the site HTML!

The both configured outputDirectory s must point to the same location- otherwise JSON reports are not picked up for further processing, not sure why....

Solution: declare either

Gradle then takes into account that there could be some other output, besides aggregate's one, and doesn't delete the entire build/site/serenity directory- aggregate picks JSONs up and produces HTML report.

P.S. thanks to @tnielens - yes i forgot to mention that this setup is not recommended by gradle - but I currently I don't see any other way to generate reports- it doesn't work otherwise...

tnielens commented 5 months ago

Gradle incremental build and task cache (to be distinguished from the configuration cache) don't work reliably if multiple tasks output results in the same directory. You'll see warnings in the log about this.

zeners commented 5 months ago

we can copy the test-result-files to a aggregate-working directory to split the test-output from the aggregate output - but this would be a breaking change

wakaleo commented 4 months ago

I'm reverting all of these changes as they have caused a number of regressions.

zeners commented 4 months ago

wich ones?

wakaleo commented 4 months ago

https://github.com/serenity-bdd/serenity-core/issues/3468

zeners commented 4 months ago

we should found a way to go with configuration cache

tnielens commented 4 months ago

@wakaleo are we back with #19 merged and the small fix #30 as well? That's what we run in my org, and it works fine, configuration cache included. Some issues with overlapping output directories for the task cache, but nothing blocking so far.

wakaleo commented 4 months ago

@wakaleo are we back with #19 merged and the small fix #30 as well?

I'm rolling back all the changes. There are too many issues being raised and I don't have time or the Gradle knowledge to deal with them.

tnielens commented 4 months ago

As said above, if you include #30, #19 works fine. I can provide support to iron out issues and increase the test coverage.

wakaleo commented 4 months ago

As said above, if you include #30, #19 works fine. I can provide support to iron out issues and increase the test coverage.

30 was included in the code base. I will roll it all back and you are welcome to redo the PRs, but we need an up to date sample project as the Gradle unit tests don't pick up all the issues.

zeners commented 4 months ago

would be nice to run tests before merged

wakaleo commented 4 months ago

would be nice to run tests before merged

To run the full test suite you need to publish the plugin - if you can find a way to run end-to-end tests with the Gradle plugin without needing to publish it that could work better.

zeners commented 4 months ago

that's that the tests currently do, no need to be published previuosly

wakaleo commented 4 months ago

that's that the tests currently do, no need to be published previuosly

There are unit tests, but since Groovy is a dynamic language they don't pick up all the issues, which only appear when you run the plugin against a real project. If you know how to write local end-to-end tests that do pick up these issues that would be a great help.

zeners commented 4 months ago

could github run these tests on PR as an action?

wakaleo commented 4 months ago

No, because if you run them against a project separately, you need to publish the plugin first which can only be done by hand. (We do run the Gradle unit tests for each build, but they are not sufficient).

tnielens commented 4 months ago

@wakaleo gradle has great support for e2e tests (they call it functional tests). Doesn't this suite fulfill your requirements?

zeners commented 4 months ago

currently tests are insufficient: accepted the only check currently run is the GitGuardian Security Checks i like to see the tests running as check on PR

wakaleo commented 4 months ago

@wakaleo gradle has great support for e2e tests (they call it functional tests). Doesn't this suite fulfill your requirements?

No it does not. We get lots of errors appearing when we actually try the tests out on a real project that don't appear in these tests.

wakaleo commented 4 months ago

currently tests are insufficient: accepted the only check currently run is the GitGuardian Security Checks i like to see the tests running as check on PR

Can you propose a PR on the Github actions to do this?

tnielens commented 4 months ago

The current test coverage in the project is gappy. We can add a lot more test cases. The multi module test suite was added to cover a particular failure scenario.

zeners commented 4 months ago

Can you propose a PR on the Github actions to do this?

i'm not familiar with github actions, will take a look, will need time, maybe someone is faster ;)

wakaleo commented 4 months ago

The current test coverage in the project is gappy. We can add a lot more test cases. The multi module test suite was added to cover a particular failure scenario.

More tests would be very welcome.

tnielens commented 4 months ago

19 added several tests which were reverted unfortunately. I suppose they can be reapplied without the main code patch easily.

tnielens commented 4 months ago

For github actions, here is some doc by gradle: https://docs.gradle.org/current/userguide/github-actions.html#sec:configure_github_actions . I'll try that out.

tnielens commented 4 months ago

There was a workflow already in place, but it didn't run on PR: https://github.com/serenity-bdd/serenity-gradle-plugin/pull/35 .

zeners commented 4 months ago

it works, the tests failed:

2024-05-22T10:41:17.7088829Z ExplicitTaskDependenciesTest > explicitDependencyBetweenClearReportsAndCheckOutcomes() FAILED
2024-05-22T10:41:17.7090682Z     org.gradle.testkit.runner.UnexpectedBuildFailure at ExplicitTaskDependenciesTest.java:78
2024-05-22T10:41:18.6085785Z 
2024-05-22T10:41:18.6086694Z MultiModuleTest > multiModulesAggregateTheReportInTheRightLocation() FAILED
2024-05-22T10:41:18.6087995Z     org.gradle.testkit.runner.UnexpectedBuildFailure at MultiModuleTest.java:94
tnielens commented 4 months ago

@zeners see the comments in the PR. The current master revision is broken it seems. The plugin cannot register certain tasks.

zeners commented 4 months ago

that have been fixed and is now rolled back?

tnielens commented 4 months ago

@wakaleo patched master. The tests now pass.

zeners commented 4 months ago

so we have now a working master and checked test on PR: thanks!

tnielens commented 4 months ago

@zeners what are your plans here. Do you intend to resubmit your changes? I'd like to resubmit the configuration cache support originally submitted in #19 but would like to avoid working on the same tasks as you.

zeners commented 4 months ago

@tnielens i would like to resubmit, after having tests for the issues, but it will take some time (may be at weekend) please feel free to start

tnielens commented 4 months ago

for my understanding, did any of your changes concern the configuration cache?

wakaleo commented 4 months ago

for my understanding, did any of your changes concern the configuration cache?

No

zeners commented 4 months ago

@tnielens any news/progress?

tnielens commented 4 months ago

No. I haven't started yet. I intended to start from the former configuration cache PR #19 plus my small fix #30. Feel free to take a head start if you wanna work on this this weekend.

tnielens commented 3 weeks ago

@zeners @wakaleo I just checked latest revision 5ee4d91 on master. I don't face any configuration cache issues. Not sure what you reverted @wakaleo but as far as I checked, the configuration cache is functional with the serenity plugin applied.