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

Check for the minimum size of the bytes read only once per frame [API-1546] #1411

Merged
merged 12 commits into from Dec 29, 2022

Conversation

fatihozer0
Copy link
Contributor

I am trying to add test cases which are added to Java (hazelcast/hazelcast#21502 (comment)) to see if there is a problem and be sure for every commit. But there is a problem which does not allow me to use already implemented methods such as getLength or getTotalLength. Can you check please?

I am aware of the name of the file. I am planning to add these test cases to file named ClientMessageReaderTest.js after the end of implementation.

@fatihozer0
Copy link
Contributor Author

I am aware of the code quality problems.
All the cases are implemented. It looks like there is not a problem like in Java. I think these should be added to ClientMessageReaderTest.js right?

@codecov
Copy link

codecov bot commented Nov 5, 2022

Codecov Report

Merging #1411 (df79912) into master (f026c81) will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1411      +/-   ##
==========================================
- Coverage   93.37%   93.34%   -0.04%     
==========================================
  Files         466      466              
  Lines       16623    16623              
  Branches     1351     1351              
==========================================
- Hits        15522    15516       -6     
- Misses        799      806       +7     
+ Partials      302      301       -1     
Impacted Files Coverage Δ
src/nearcache/NearCache.ts 92.24% <0.00%> (-1.56%) ⬇️
src/proxy/NearCachedMapProxy.ts 93.05% <0.00%> (-1.39%) ⬇️
src/network/ConnectionManager.ts 83.99% <0.00%> (-1.17%) ⬇️
src/core/HazelcastError.ts 77.61% <0.00%> (ø)
src/protocol/ErrorFactory.ts 64.66% <0.00%> (ø)
src/invocation/InvocationService.ts 94.87% <0.00%> (ø)
src/util/Util.ts 87.67% <0.00%> (+0.68%) ⬆️
...rc/serialization/portable/DefaultPortableReader.ts 85.00% <0.00%> (+1.42%) ⬆️

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

@yuce yuce changed the title Add test cases to see if there is a problem or not [API-1546] Check for the minimum size of the bytes read only once per frame [API-1546] Nov 7, 2022
@srknzl
Copy link
Member

srknzl commented Nov 7, 2022

@fatihozer0 Sure. You can add to the ClientMessageReaderTest.

test/unit/connection/ClientMessageReaderTestRenewed.js Outdated Show resolved Hide resolved
test/unit/connection/ClientMessageReaderTestRenewed.js Outdated Show resolved Hide resolved
test/unit/connection/ClientMessageReaderTestRenewed.js Outdated Show resolved Hide resolved
test/unit/connection/ClientMessageReaderTestRenewed.js Outdated Show resolved Hide resolved
test/unit/connection/ClientMessageReaderTestRenewed.js Outdated Show resolved Hide resolved
test/unit/connection/ClientMessageReaderTestRenewed.js Outdated Show resolved Hide resolved
test/unit/connection/ClientMessageReaderTestRenewed.js Outdated Show resolved Hide resolved
test/unit/connection/ClientMessageReaderTestRenewed.js Outdated Show resolved Hide resolved
test/unit/connection/ClientMessageReaderTestRenewed.js Outdated Show resolved Hide resolved
test/unit/connection/ClientMessageReaderTestRenewed.js Outdated Show resolved Hide resolved
@srknzl
Copy link
Member

srknzl commented Nov 7, 2022

@fatihozer0 Please add milestone, label etc. to the PR

@fatihozer0 fatihozer0 self-assigned this Nov 15, 2022
@fatihozer0 fatihozer0 added this to the 5.2.0 milestone Nov 15, 2022
test/unit/connection/ClientMessageReaderTest.js Outdated Show resolved Hide resolved
test/unit/connection/ClientMessageReaderTest.js Outdated Show resolved Hide resolved
test/unit/connection/ClientMessageReaderTest.js Outdated Show resolved Hide resolved
test/unit/connection/ClientMessageReaderTest.js Outdated Show resolved Hide resolved
test/unit/connection/ClientMessageReaderTest.js Outdated Show resolved Hide resolved
@fatihozer0
Copy link
Contributor Author

All review comments are applied.

@srknzl
Copy link
Member

srknzl commented Nov 21, 2022

@fatihozer0 All comments are not applied see my responses in unresolved comments

srknzl
srknzl previously approved these changes Nov 28, 2022
srknzl
srknzl previously approved these changes Dec 13, 2022
@harunalpak harunalpak modified the milestones: 5.2.0, 5.3.0 Dec 14, 2022
@fatihozer0 fatihozer0 merged commit 7c4f088 into hazelcast:master Dec 29, 2022
srknzl pushed a commit to fatihozer0/hazelcast-nodejs-client that referenced this pull request Mar 24, 2023
…-1546] (hazelcast#1411)

* add test cases to see if there is a problem or not

* add all the test cases of Java

* add test cases to ClientMessageReaderTest.js from ClientMessageReaderTestRenewed.js and delete ClientMessageReaderTestRenewed.js.

* add test cases to ClientMessageReaderTest.js from ClientMessageReaderTestRenewed.js and delete ClientMessageReaderTestRenewed.js.

* add test comments

* separate the test describers

* fix some typos

* fix some typos

* fix a minor problem

* fix the ubuntu-test problem

* fix some lint problems
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants