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.

Sign up for GitHub

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

Jump to bottom

Fix CPSessionManagerTest #1164

Merged
merged 1 commit into from
Jan 6, 2022
Merged

Fix CPSessionManagerTest #1164

merged 1 commit into from
Jan 6, 2022

Conversation

srknzl
Copy link
Contributor

@srknzl srknzl commented Jan 6, 2022 •
edited
Loading

We forgot to change this test in #797. Invocation service is used in heartbeatTask in CPSessionManager and leads to an error. I fixed the initialization of CPSessionManager. The test was flaky, and the situation this test passes is probably when it passes before the first heartbeat task runs.

closes #1022

@srknzl srknzl added this to the 5.1 milestone Jan 6, 2022
@srknzl srknzl requested review from yuce and mdumandag January 6, 2022 10:40
@codecov
Copy link

codecov bot commented Jan 6, 2022

Codecov Report

Merging #1164 (2396c9d) into master (cd6240e) will decrease coverage by 0.43%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1164      +/-   ##
==========================================
- Coverage   92.53%   92.10%   -0.44%     
==========================================
  Files         438      438              
  Lines       14521    14521              
  Branches     1085     1085              
==========================================
- Hits        13437    13374      -63     
- Misses        795      852      +57     
- Partials      289      295       +6
Impacted Files Coverage Δ
src/codec/ClientAuthenticationCustomCodec.ts 42.22% <0.00%> (-33.34%) ⬇️
src/network/ConnectionManager.ts 75.17% <0.00%> (-8.52%) ⬇️
src/protocol/ErrorFactory.ts 62.40% <0.00%> (-2.26%) ⬇️
src/core/HazelcastError.ts 73.98% <0.00%> (-1.63%) ⬇️
src/config/ConfigBuilder.ts 90.13% <0.00%> (-1.10%) ⬇️
src/invocation/ClusterService.ts 93.16% <0.00%> (-0.86%) ⬇️
src/util/Util.ts 87.89% <0.00%> (-0.64%) ⬇️
src/invocation/InvocationService.ts 93.80% <0.00%> (-0.45%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd6240e...2396c9d. Read the comment docs.

@mdumandag
Copy link
Contributor

Can we catch such errors, if we were to use typescript in the tests as well? Maybe it would be good idea to invest some time in this area after 5.1

@srknzl
Copy link
Contributor Author

srknzl commented Jan 6, 2022

Good idea. We could use https://www.npmjs.com/package/ts-mocha to run our tests in the future if we switch our tests to typescript. This library only transpiles does not do type checking(still IDEs will show errors). To do also type checking, we can compile test files in npm run compile command.

@srknzl srknzl merged commit 818fef6 into hazelcast:master Jan 6, 2022
harunalpak pushed a commit to harunalpak/hazelcast-nodejs-client that referenced this pull request Dec 8, 2022
@srknzl
Fix CPSessionManagerTest (hazelcast#1164)
eb61921 
We forgot to change this test in hazelcast#797. Invocation service is used in heartbeatTask in CPSessionManager and leads to an error. I fixed the initialization of CPSessionManager. The test was flaky, and the situation this test passes is probably when it passes before the first heartbeat task runs.

closes hazelcast#1022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuce yuce yuce approved these changes

@mdumandag mdumandag mdumandag approved these changes

Assignees
No one assigned
Labels
Estimation: S Type: Test-Failure
Projects
None yet
Milestone
5.1.0
Development

Successfully merging this pull request may close these issues.

CPSessionManagerTest fail on windows [API-1300]
3 participants
@srknzl @mdumandag @yuce