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

chore: correct incorrect comment #268

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

diaohancai
Copy link
Contributor

Purpose of the PR

Main Changes

Correct incorrect comment.

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

@diaohancai diaohancai changed the title fix #267 fix: Incorrect comment Sep 17, 2023
@diaohancai diaohancai changed the title fix: Incorrect comment chore: Incorrect comment Sep 17, 2023
@diaohancai diaohancai changed the title chore: Incorrect comment chore: Correct incorrect comment Sep 17, 2023
@diaohancai diaohancai changed the title chore: Correct incorrect comment chore: correct incorrect comment Sep 17, 2023
@imbajin imbajin requested a review from coderzc September 17, 2023 08:36
Copy link
Member

@simon824 simon824 left a comment

Choose a reason for hiding this comment

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

LGTM

@simon824
Copy link
Member

By the way, hugegraph-computer ci was failed and it seems not to be caused by this PR. can u help to improve it when free? (i have rerun many times) @coderzc @Radeity

@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Merging #268 (ec06513) into master (fdb4621) will increase coverage by 0.05%.
Report is 1 commits behind head on master.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master     #268      +/-   ##
============================================
+ Coverage     85.83%   85.89%   +0.05%     
- Complexity     3235     3236       +1     
============================================
  Files           344      344              
  Lines         12115    12115              
  Branches       1092     1092              
============================================
+ Hits          10399    10406       +7     
+ Misses         1194     1187       -7     
  Partials        522      522              
Files Changed Coverage Δ
.../hugegraph/computer/core/common/ContainerInfo.java 90.90% <ø> (ø)

... and 2 files with indirect coverage changes

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

@simon824 simon824 merged commit 15fc9bb into apache:master Sep 20, 2023
7 checks passed
@Radeity
Copy link
Member

Radeity commented Sep 20, 2023

By the way, hugegraph-computer ci was failed and it seems not to be caused by this PR. can u help to improve it when free? (i have rerun many times) @coderzc @Radeity

I've checked error logs and find that there's an unexpected state change, we expect to receive CANCELLED, but receive some states else. And may I ask why we have to sleep 1.5s after calling cancelJob because state may be changed to CANCELLED in this 1.5s and we will miss the state change in UT. cc @coderzc

this.driver.cancelJob(jobId, params);
UnitTestBase.sleep(1500L);
DefaultJobState jobState2 = new DefaultJobState();
jobState2.jobStatus(JobStatus.CANCELLED);
Mockito.verify(jobObserver, Mockito.timeout(15000L).atLeast(1))
.onJobStateChanged(Mockito.eq(jobState2));

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] Incorrect code comment
4 participants