RFR: Fine-tune testing on Windows

Tue Aug 27 13:58:46 UTC 2019

On Tue, 27 Aug 2019 13:43:26 GMT, Christian Stein via github.com <duke at openjdk.java.net> wrote:

> On Tue, 27 Aug 2019 13:11:16 GMT, Erik Duveblad via github.com <duke at openjdk.java.net> wrote:
> 
>> On Tue, 27 Aug 2019 12:51:56 GMT, Christian Stein via github.com <duke at openjdk.java.net> wrote:
>> 
>>> On Tue, 27 Aug 2019 12:06:55 GMT, Erik Duveblad via github.com <duke at openjdk.java.net> wrote:
>>> 
>>>> On Tue, 27 Aug 2019 08:52:05 GMT, Christian Stein via github.com <duke at openjdk.java.net> wrote:
>>>> 
>>>>> On Tue, 27 Aug 2019 08:45:46 GMT, Jorn Vernee via github.com <duke at openjdk.java.net> wrote:
>>>>> 
>>>>>> On Mon, 26 Aug 2019 18:52:18 GMT, Christian Stein via github.com <duke at openjdk.java.net> wrote:
>>>>>> 
>>>>>>> ### Two more Windows issues down
>>>>>>> 
>>>>>>> - **Skip BrideBot tests on Windows**
>>>>>>> The `Exporter` class, used `JBridgeBot`, calls application `"rsync"`, which isn't available on Windows per default. Therefore, the annotation `@DisabledOnOS(OS.WINDOWS)` is attached to the entire test class.
>>>>>>> Due to `@BeforeAll void setup()` (and `teardown()`) being called anyway, a second gatekeeper in form an assumption is added to this method as well. This improves the execution duration time of all disabled tests.
>>>>>>> 
>>>>>>> - **Convert relative folder path to be URI-friendly**
>>>>>>> On Windows Path::toString generates back slashes as path element separators. This yields invalid URI paths: like `"%s\webrev.%s"`.
>>>>>>> 
>>>>>>> ### Open ends
>>>>>>> 
>>>>>>> Current show stopper on Windows is:
>>>>>>> 
>>>>>>> ```java.io.UncheckedIOException: java.io.IOException: Unexpected exit code
>>>>>>> 'git commit --message=Added webrev' exited with status: 1
>>>>>>> [stdout]
>>>>>>>> HEAD detached at b6bb82c
>>>>>>>> nothing to commit, working tree clean
>>>>>>> [stderr]
>>>>>>> 	at org.openjdk.skara.bots.mlbridge/org.openjdk.skara.bots.mlbridge.WebrevStorage.createAndArchive(WebrevStorage.java:90)
>>>>>>> 	at org.openjdk.skara.bots.mlbridge/org.openjdk.skara.bots.mlbridge.ArchiveWorkItem.run(ArchiveWorkItem.java:595)
>>>>>>> 	at org.openjdk.skara.test/org.openjdk.skara.test.TestBotRunner.runPeriodicItems(TestBotRunner.java:34)
>>>>>>> 	at org.openjdk.skara.bots.mlbridge/org.openjdk.skara.bots.mlbridge.MailingListBridgeBotTests.skipAddingExistingWebrev(MailingListBridgeBotTests.java:800)
>>>>>>> 
>>>>>>> ...
>>>>>>> 
>>>>>>> Caused by: java.io.IOException: Unexpected exit code
>>>>>>> 'git commit --message=Added webrev' exited with status: 1
>>>>>>> [stdout]
>>>>>>>> HEAD detached at b6bb82c
>>>>>>>> nothing to commit, working tree clean
>>>>>>> [stderr]
>>>>>>> 	at org.openjdk.skara.vcs/org.openjdk.skara.vcs.git.GitRepository.await(GitRepository.java:94)
>>>>>>> 	at org.openjdk.skara.vcs/org.openjdk.skara.vcs.git.GitRepository.commit(GitRepository.java:556)
>>>>>>> 	at org.openjdk.skara.vcs/org.openjdk.skara.vcs.git.GitRepository.commit(GitRepository.java:521)
>>>>>>> 	at org.openjdk.skara.vcs/org.openjdk.skara.vcs.git.GitRepository.commit(GitRepository.java:516)
>>>>>>> 	at org.openjdk.skara.bots.mlbridge/org.openjdk.skara.bots.mlbridge.WebrevStorage.push(WebrevStorage.java:60)
>>>>>>> 	at org.openjdk.skara.bots.mlbridge/org.openjdk.skara.bots.mlbridge.WebrevStorage.createAndArchive(WebrevStorage.java:86)
>>>>>>> ```
>>>>>>> 
>>>>>>> ----------------
>>>>>>> 
>>>>>>> Commits:
>>>>>>>  - 91ea9193:	Convert relative folder path to be URI-friendly
>>>>>>> 			On Windows Path::toString generates back slashes as path element
>>>>>>> 			separators. This yields invalid URI paths: like "%s\webrev.%s".
>>>>>>>  - 7bc47b57:	Skip BridgeBot tests on Windows
>>>>>>> 			The Exporter class, used JBridgeBot, calls application "rsync", which
>>>>>>> 			isn't available on Windows per default. Therefore, the annotation
>>>>>>> 			`@DisabledOnOS(OS.WINDOWS)` is attached to the entire test class.
>>>>>>> 			Due to `@BeforeAll void setup()` (and teardown()) being called anyway,
>>>>>>> 			a second gatekeeper in form an assumption is added to this method as
>>>>>>> 			well. This improves the execution duration time of all disabled tests.
>>>>>>> 
>>>>>>> Pull request:
>>>>>>> https://git.openjdk.java.net/skara/pull/86
>>>>>>> 
>>>>>>> Webrev:
>>>>>>> https://webrevs.openjdk.java.net/skara/86/webrev.00
>>>>>>> 
>>>>>>> Patch:
>>>>>>> https://git.openjdk.java.net/skara/pull/86.diff
>>>>>>> 
>>>>>>> Fetch command:
>>>>>>> git fetch https://git.openjdk.java.net/skara pull/86/head:pull/86
>>>>>> 
>>>>>> Thanks for looking into this! With your patch all the `mlbridge` tests are passing for me. I'm not seeing the same failures as you with the `mlbridge` tests...
>>>>>> 
>>>>>> I'm not a Skara reviewer, so I can't approve this, but the patch looks good to me ðŸ‘
>>>>>> 
>>>>>> ---
>>>>>> FWIW, I also got some other failures from some of the bot tests, particularly `IntegrateTests::autorebase`, `MergeTests::branchMergeRebase` and `SponsorTests::autoRebase` in the `bots.pr` module. When looking into it this seemed to be caused by file paths exceeding the maximum path length on Windows. Git rejects them with a specific error message, and HG just throws a file not found error.
>>>>>> 
>>>>>> Besides that some tests in the `bots.submit` module are failing as well, but I haven't determined the exact reason yet.
>>>>>> 
>>>>>> I had a chat with Erik about this, and since the bots are not really required to run on Windows any ways, it would also be fine to disable all the `bots` tests from running on Windows, to avoid any further compatibility problems. Or, just aggressively disable any that are failing.
>>>>>> 
>>>>>> PR: https://git.openjdk.java.net/skara/pull/86
>>>>> 
>>>>>> [...] it would also be fine to disable all the bots tests from running on Windows [...]
>>>>> 
>>>>> As long as some CI service is running those tests on Linux/Mac, fine with me.
>>>>> 
>>>>> I'll extend this PR with to disable all bots tests on Windows.
>>>>> 
>>>>> PR: https://git.openjdk.java.net/skara/pull/86
>>>> 
>>>> This looks good, but @sormuras would you mind updating the title of the PR? The title of the PR will be used as the title for the resulting commit, and I think we would want something a bit more descriptive than `Windows again...` :smile:
>>>> 
>>>> PR: https://git.openjdk.java.net/skara/pull/86
>>> 
>>> Sure, Erik.
>>> 
>>> But I'll update the PR once more to include comments from above:
>>> - Disable all bots test
>>> - Revert not needed optimization -- Gradle does just fine, only IDEA executes disabled tests.
>>>> Due to @BeforeAll void setup() (and teardown()) being called anyway, a second gatekeeper in form an assumption is added to this method as well. This improves the execution duration time of all disabled tests.
>>> 
>>> PR: https://git.openjdk.java.net/skara/pull/86
>> 
>> Ok, sounds good!
>> 
>> PR: https://git.openjdk.java.net/skara/pull/86
> 
> Sneak-peek: https://gradle.com/s/u4a6jzvrpkcvs
> 
> PR: https://git.openjdk.java.net/skara/pull/86

Instead of annotating each test class in every `bots` project, I implemented an `ExecutionCondition` and enabled auto-detection of provided Jupiter extension. That implementation is named `DisableAllBotsTestsOnWindows` and resides in the `org.openjdk.skara.test` module. It checks the current test classes' package name. If that package name starts with `"org.openjdk.skara.bots."` and the current operating system is Windows, the execution of all tests within the test class are disabled.

Too much magic?

PR: https://git.openjdk.java.net/skara/pull/86