Skip to content

Conversation

@sungshik
Copy link
Contributor

@sungshik sungshik commented Apr 14, 2025

In #20, a number of tests were added for overflow handling. The bookkeeping code to keep track of actual events vs. expected events in those tests had two issues: (a) two tests still used counting of events, which can be unreliable; (b) a few other tests had roughly the same bookkeeping code duplicated.

This PR fixes these issues by adding a general Bookkeeping class that is: (a) not based on counting; (b) reused across tests.

@codecov
Copy link

codecov bot commented Apr 14, 2025

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 81.0%. Comparing base (5e7da8e) to head (c201bb7).
Report is 26 commits behind head on main.

Files with missing lines Patch % Lines
...c/main/java/engineering/swat/watch/WatchEvent.java 66.6% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##              main     #35     +/-   ##
=========================================
+ Coverage     80.9%   81.0%   +0.1%     
- Complexity     132     136      +4     
=========================================
  Files           16      16             
  Lines          567     570      +3     
  Branches        64      66      +2     
=========================================
+ Hits           459     462      +3     
+ Misses          69      68      -1     
- Partials        39      40      +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sungshik sungshik marked this pull request as draft April 14, 2025 10:32
Copy link
Contributor Author

@sungshik sungshik left a comment

Choose a reason for hiding this comment

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

Comments

Comment on lines -170 to -172
// Preface: This test looks a bit hacky because there's no API to
// directly manipulate, or prevent the auto-manipulation of, the index
// inside a watch. I've added some comments below to make it make sense.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test was originally written before we added the filter mechanism to simulate overflows. Now that we have that, this test can be simplified a lot (without the hacks).

Comment on lines -185 to -201
private static class Bookkeeper implements Consumer<WatchEvent> {
private final List<WatchEvent> events = Collections.synchronizedList(new ArrayList<>());

public Stream<WatchEvent> events(WatchEvent.Kind... kinds) {
var list = Arrays.asList(kinds.length == 0 ? WatchEvent.Kind.values() : kinds);
return events.stream().filter(e -> list.contains(e.getKind()));
}

public Stream<Path> fullPaths(WatchEvent.Kind... kinds) {
return events(kinds).map(WatchEvent::calculateFullPath);
}

@Override
public void accept(WatchEvent e) {
events.add(e);
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This class was moved to TestHelper, so it can be reused across test classes. (Actually, after moving it, the class was rewritten quite a bit, but the idea remained the same.)

@sungshik sungshik marked this pull request as ready for review April 14, 2025 13:25
@sungshik sungshik requested a review from DavyLandman April 14, 2025 13:26
Copy link
Member

@DavyLandman DavyLandman left a comment

Choose a reason for hiding this comment

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

Just a few small things, looks good.

@sungshik sungshik merged commit d97a184 into main Apr 14, 2025
15 checks passed
@sungshik sungshik deleted the refactor-tests branch April 14, 2025 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants