split code and tests in a PR

[Kevin Rushforth]

When I am doing a review and want to test a PR, I do something similar 
to what Johan mentioned. I download the PR to my local fork, verify it 
(often with manual testing in addition to the provided tests), and then 
backout the code changes to see whether the provided automated test 
would catch the failure.

The Skara bots don't run any tests at all. What Skara does do is run a 
PR check that looks for the results of a GitHub actions test run, which 
is run in the context of the personal fork of the PR author, and present 
those results to the reviewers of the PR. So we already get this level 
of testing.

I can't think of a good way to automate running the new / modified tests 
without the fix. I don't think we want Skara to get into the business of 
understanding enough about the different projects (jfx, jdk, skara 
itself, etc) to be able to split a PR into multiple parts. Also, we 
don't run any tests either in the bots themselves or in the openjdk 
group on GitHub, only on individual user's personal forks. Anything we 
do would be down to whatever we could run in the GitHub actions on the 
developer's personal fork.

-- Kevin


On 6/22/2021 9:06 AM, Nir Lisker wrote:
> I also thought it would be useful for the bot to run the tests after the
> patch and notify on a failure. I didn't think about applying the new tests
> to the old code with "hopes" for a failure, I like this idea. Providing 2
> downloads is also useful.
>
> One thing we need to be careful with is that an older test can fail with a
> new patch even though the tests that come with the patch pass because the
> patch could have broken some dependent code (which could also mean that the
> problem is in the dependent code).
> How expensive is it for the bot to run the tests?
>
> On Tue, Jun 22, 2021 at 6:31 PM Johan Vos <johan.vos at gluonhq.com> wrote:
>
>> Hi,
>>
>> I really like the automation that Skara is providing. It saves time for
>> committers as well as for reviewers. In order to increase productivity even
>> more (without decreasing quality), there might be an additional step we
>> might somehow automate. Many PR's contain changes in java code and in test
>> code. What I typically do as a first step of a review, is to first run the
>> tests on the latest code, then apply the PR, and run the tests again.
>> Clearly, no test should fail in the second run, while it would be ok (and
>> desired) that at least 1 tests fails in the first run.
>>
>> My typical approach for this is to download the diff, and manually split it
>> in 2 parts: code + tests. I then apply the part with the tests, and run
>> them. Next, I apply the part with the code, and re-run the tests.
>> Clearly, this is not enough for a review, but if we automate this, it gives
>> an additional indication that a PR is going in the right direction.
>>
>> I'm not sure how easy/hard this would be to implement. The bot should be
>> able to detect the `src/test/java`, and provide 2 downloads in the comments
>> of the PR ("download test diff" and "download code diff"). It can also
>> execute those steps in a github action, and fail if the resulting tests are
>> failing after applying the code diff.
>>
>> Thoughts?
>>
>> - Johan
>>