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(algorithm): record loop is not copied #276

Merged
merged 1 commit into from
Oct 28, 2023
Merged

Conversation

corgiboygsj
Copy link
Member

Purpose of the PR

Main Changes

The result is not copied when the loop is detected.

Verifying these changes

  • Trivial rework / code cleanup without any test coverage. (No Need)
  • Already covered by existing tests, such as (please modify tests here).
  • Need tests and can be verified as follows.

Does this PR potentially affect the following parts?

  • Nope
  • Dependencies (add/update license info)
  • Modify configurations
  • The public API
  • Other affects (typed here)

Documentation Status

  • Doc - TODO
  • Doc - Done
  • Doc - No Need

@corgiboygsj corgiboygsj requested a review from javeme October 25, 2023 12:57
@codecov
Copy link

codecov bot commented Oct 25, 2023

Codecov Report

Merging #276 (35364c2) into master (2e45cd2) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #276      +/-   ##
============================================
- Coverage     85.10%   85.09%   -0.02%     
- Complexity     3245     3246       +1     
============================================
  Files           345      345              
  Lines         12283    12283              
  Branches       1101     1101              
============================================
- Hits          10454    10452       -2     
- Misses         1303     1304       +1     
- Partials        526      527       +1     
Files Coverage Δ
.../computer/algorithm/path/rings/RingsDetection.java 87.50% <100.00%> (ø)

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -124,6 +125,10 @@ private void assertResult(Id id, List<String> rings) {
Set<String> expect = EXPECT_RINGS.getOrDefault(id.toString(),
new HashSet<>());

rings = rings.stream()
.distinct()
.collect(Collectors.toList());
Copy link
Contributor

Choose a reason for hiding this comment

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

seems don't need to distinct since the rings is a Set

Copy link
Member Author

Choose a reason for hiding this comment

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

seems don't need to distinct since the rings is a Set

rings is a List.

private void assertResult(Id id, List<String> rings) {
   ...
}

Before fix this bug, it is also a problem that our test can pass.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry

Copy link
Contributor

@javeme javeme left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -124,6 +125,10 @@ private void assertResult(Id id, List<String> rings) {
Set<String> expect = EXPECT_RINGS.getOrDefault(id.toString(),
new HashSet<>());

rings = rings.stream()
.distinct()
.collect(Collectors.toList());
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry

@imbajin imbajin merged commit 85e6399 into master Oct 28, 2023
7 of 8 checks passed
@imbajin imbajin deleted the fix-ring-detection-result branch October 28, 2023 16:05
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.

[Bug] RingsDetection test did not meet expectations
3 participants