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

Extend Karate tests for decentralized communication #1702

Merged
merged 12 commits into from
Jan 4, 2024

Conversation

simone-kalbermatter
Copy link
Contributor

@simone-kalbermatter simone-kalbermatter commented Oct 15, 2023

This PR:

  • Refactors JsonConverter and MultiMsgWebSocketClient to make them easier to understand,
  • Makes message ids of publish messages accessible in feature files. This improves checks on the content of messages returned by the tested components and can be used to craft better test messages.
  • Adds a test case for the getMessagesById message that checks that a server does not request messages in a hearbeat that it already received.

@simone-kalbermatter simone-kalbermatter self-assigned this Oct 15, 2023
@github-actions
Copy link

Pull reviewers stats

Stats of the last 30 days for popstellar:

User Total reviews Time to review Total comments
pierluca
🥇
4
▀▀▀▀
2d 8h 32m
16
▀▀▀▀▀▀▀▀
K1li4nL
🥈
3
▀▀▀
3d 14h 9m
0
jbsv
🥉
2
▀▀
8d 1h 46m
▀▀▀
4
▀▀
simone-kalbermatter
1
9d 10h 40m
▀▀▀
1
matteosz
1
5d 2h 48m
▀▀
0

@simone-kalbermatter simone-kalbermatter added the prod-ready Production Ready related pull requests label Oct 17, 2023
@simone-kalbermatter simone-kalbermatter marked this pull request as ready for review November 27, 2023 18:14
@simone-kalbermatter simone-kalbermatter requested a review from a team as a code owner November 27, 2023 18:14
Copy link
Contributor

@pierluca pierluca left a comment

Choose a reason for hiding this comment

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

LGTM.
Some elements could be improved here and there (e.g. more heartbeat tests / more complex scenarios), but as a foundation, this is already pretty nice 👍

Comment on lines 51 to 52
public void send(Map<String, Object> messageData){
this.send(mapToJsonString(messageData));
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd consider renaming messageData to something like wsMessage or jsonRpcMsg to indicate that we're talking about the lowest level.
messageData is already pretty (over)loaded as a concept in this project.

@simone-kalbermatter
Copy link
Contributor Author

@pierluca Yes, this is only a foundation so far. I realized that splitting the server tests from the client tests was probably not the best idea for more complex test scenarios 😅 I might add them back together, but I want to do this in a seperate PR.

@matteosz matteosz self-requested a review December 2, 2023 10:17
Copy link
Contributor

@matteosz matteosz left a comment

Choose a reason for hiding this comment

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

LGTM, just few comments

Comment on lines +84 to +85
And def messageIds = []
And eval messageIds.push(message_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to initialize in-line?
And def messageIds = [message_id]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like Karate has some problem parsing this as a JSON array, I get an error if I try to do it this way.

Comment on lines 27 to 28

Then assert heartbeatMessages.length > 0
Then assert heartbeatMessages.length > 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be better to check for the actual number? Or at least if we cannot determine it precisely we could add an upper threshold (just to be sure heartbeats are not spammed more than expected)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking to keep it more open in case it changes in the future, but right now it can absolutely just check that the number is 2.

*/
public void publish(Map<String, Object> highLevelMessageDataMap, String channel){
String highLevelMessageData = mapToJsonString(highLevelMessageDataMap);
int messageId = new Random().nextInt(Integer.MAX_VALUE);
Copy link
Contributor

Choose a reason for hiding this comment

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

Integer.MAX_VALUE is already the default max value for nextInt so it's redundant


private PublishMessageIds getPublishMessageIds(Map<String, Object> highLevelMessageDataMap){
String highLevelMessageData = mapToJsonString(highLevelMessageDataMap);
PublishMessageIds message_ids = sentMessages.get(highLevelMessageData);
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: I'm not a great fan of snake case in Java 😬

Copy link

sonarcloud bot commented Dec 5, 2023

[PoP - PoPCHA-Web-Client] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link

sonarcloud bot commented Dec 5, 2023

[PoP - Be1-Go] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link

sonarcloud bot commented Dec 5, 2023

[PoP - Be2-Scala] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link

sonarcloud bot commented Dec 5, 2023

[PoP - Fe2-Android] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link

sonarcloud bot commented Dec 5, 2023

[PoP - Fe1-Web] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Contributor

@pierluca pierluca left a comment

Choose a reason for hiding this comment

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

LGTM.
Are you satisfied @matteosz ?

@matteosz matteosz self-requested a review January 3, 2024 10:23
Copy link
Contributor

@matteosz matteosz left a comment

Choose a reason for hiding this comment

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

LGTM!

@simone-kalbermatter simone-kalbermatter merged commit 482838b into master Jan 4, 2024
18 checks passed
@simone-kalbermatter simone-kalbermatter deleted the work-simone-extend-decentralized-tests branch January 4, 2024 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prod-ready Production Ready related pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants