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

Disable file watcher in VcsManagerTest suite #7421

Merged
merged 3 commits into from
Jul 31, 2023
Merged

Conversation

4e6
Copy link
Contributor

@4e6 4e6 commented Jul 28, 2023

Pull Request Description

We can't really control the timing of file watcher events, which sometimes leads to failures of VCS tests. The PR disables the file watcher in the VcsManagerTest suite to make tests more stable.

Changelog:

  • add: a Watcher and WatcherFactory interfaces
  • add: a NoopWatcher test watcher
  • update: disable the file watcher in the VcsManagerTest suite

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.

@4e6 4e6 added the CI: No changelog needed Do not require a changelog entry for this PR. label Jul 28, 2023
@4e6 4e6 self-assigned this Jul 28, 2023
Copy link
Collaborator

@hubertp hubertp left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

My wish is to cleanup the dependencies of our project. I believe there should be API (e.g. Watcher) visible to the rest of the code base, but the implementation should be in a different project.

My other wish: new files (Watcher, Watcher.Factory) would better be written in Java.


import java.nio.file.Path

trait Watcher {
Copy link
Member

@JaroslavTulach JaroslavTulach Jul 31, 2023

Choose a reason for hiding this comment

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

This PR is a nice opportunity to separate API (e.g. Watcher) from the implementation and have them in different projects.

errorCallback: WatcherAdapter.WatcherError => Unit
) extends DirectoryChangeListener {

import WatcherAdapter._
Copy link
Member

Choose a reason for hiding this comment

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

The test shouldn't even see this class and definitely not io.methvin.watcher classes on the classpath!


import java.nio.file.Path

trait WatcherFactory {
Copy link
Member

Choose a reason for hiding this comment

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

I'd make the WatcherFactory an inner class of Watcher if I was designing the API in Java.

@4e6 4e6 added the CI: Ready to merge This PR is eligible for automatic merge label Jul 31, 2023
@mergify mergify bot merged commit c629d60 into develop Jul 31, 2023
@mergify mergify bot deleted the wip/db/flaky-vcs-tests branch July 31, 2023 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants