Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix AbstractScriptFileWatcher for windows #3388

Merged
merged 2 commits into from
Feb 20, 2023
Merged

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented Feb 19, 2023

Fixes #3386

This fixes a real bug with script start-level handling on windows systems.

@J-N-K J-N-K added bug An unexpected problem or unintended behavior of the Core regression labels Feb 19, 2023
@J-N-K J-N-K requested a review from a team as a code owner February 19, 2023 18:24
@J-N-K
Copy link
Member Author

J-N-K commented Feb 19, 2023

Is there any reason why we don't have the Windows GHA builds for this repo? I guess it's less important for openhab-addons since most file-handling is done in core, but I would appreciate it here.

@J-N-K
Copy link
Member Author

J-N-K commented Feb 19, 2023

GHA failed unrelated.

@wborn
Copy link
Member

wborn commented Feb 20, 2023

Thanks for having a look again.

It seems there is still one failing test, see: https:/wborn/openhab-core/actions/runs/4217807817/jobs/7329491804

[ERROR] Tests run: 21, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 19.773 s <<< FAILURE! - in org.openhab.core.automation.module.script.rulesupport.loader.AbstractScriptFileWatcherTest
[ERROR] org.openhab.core.automation.module.script.rulesupport.loader.AbstractScriptFileWatcherTest.testScriptEngineRemovedOnFailedLoad  Time elapsed: 1.156 s  <<< FAILURE!
Wanted but not invoked:
scriptEngineManagerMock.createScriptEngine(
    "js",
    "C:\Users\RUNNER~1\AppData\Local\Temp\junit1841477773041771757\script.js"
);
-> at org.openhab.core.automation.module.script.rulesupport.loader.AbstractScriptFileWatcherTest.testScriptEngineRemovedOnFailedLoad(AbstractScriptFileWatcherTest.java:588)

However, there was exactly 1 interaction with this mock:
scriptEngineManagerMock.addFactoryChangeListener(
    org.openhab.core.automation.module.script.rulesupport.loader.AbstractScriptFileWatcherTest$1@7ce4de34
);
-> at org.openhab.core.automation.module.script.rulesupport.loader.AbstractScriptFileWatcher.<init>(AbstractScriptFileWatcher.java:108)

I still have adding macOS/Windows GHA builds on my todo list. The macOS build is a lot faster now that #3004 got merged.

Whenever I trigger Windows builds, they often fail due to all the unstable tests. But I guess those issues will get more attention once Windows is added to the matrix. 😉

Signed-off-by: Jan N. Klug <[email protected]>
@J-N-K
Copy link
Member Author

J-N-K commented Feb 20, 2023

I guess this really is an unstable test (it did not fail on my local builds). Since all the tests use 10s timeout and this one uses 1s, most probably the timeout is too short with parallel builds on low power machines.

Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It builds successfully on Windows now! 👍

@wborn wborn merged commit c739c85 into openhab:main Feb 20, 2023
@wborn wborn added this to the 4.0 milestone Feb 20, 2023
@J-N-K J-N-K deleted the bug-asfw branch February 20, 2023 15:43
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of the Core regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AbstractScriptFileWatcherTest fails on Windows
2 participants