Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Add/Update events unit tests #9173

Closed
wants to merge 2 commits into from

Conversation

mosmartin
Copy link
Contributor

What was the problem?

This PR resolves #8066

How was it solved?

Added new and updated existing unit tests

How was it tested?

Added new and updated existing unit tests

Update the event name to conform to alphanumeric
@mosmartin mosmartin requested review from shuse2 and ricott1 December 4, 2023 12:20
@mosmartin mosmartin self-assigned this Dec 4, 2023
Copy link

codecov bot commented Dec 4, 2023

Codecov Report

Merging #9173 (62a3726) into release/6.1.0 (f641009) will decrease coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                @@
##           release/6.1.0    #9173      +/-   ##
=================================================
- Coverage          84.65%   84.65%   -0.01%     
=================================================
  Files                655      655              
  Lines              24113    24113              
  Branches            3497     3497              
=================================================
- Hits               20414    20413       -1     
- Misses              3699     3700       +1     

see 1 file with indirect coverage changes

@shuse2 shuse2 changed the base branch from release/6.1.0 to development December 6, 2023 21:39
@@ -82,14 +100,38 @@ describe('event', () => {
expect(topicIndex).toEqual(i);
}
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we already have a test on the number of topics? I.e. testing that the length of the topics array is <= MAX_TOPICS_PER_EVENT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has not been implemented in the file under test.

Copy link
Contributor

@ricott1 ricott1 left a comment

Choose a reason for hiding this comment

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

I suggested another test on the opics array. Furthermore, I would also add tests for the default topics (I couldn't find them in this file) and tests for exceeding maximum number of events per block (but maybe that's more an integration test).

@mosmartin
Copy link
Contributor Author

Created a new issue to address this.

@mosmartin mosmartin closed this Dec 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review of unit test for 'events'
2 participants