-
Notifications
You must be signed in to change notification settings - Fork 89
Adding unit tests to improve the code coverage of jest unit tests in ZAP #1652
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
base: master
Are you sure you want to change the base?
Adding unit tests to improve the code coverage of jest unit tests in ZAP #1652
Conversation
Summary of ChangesHello @brdandu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on a substantial effort to enhance the overall quality and reliability of the ZAP application by adding a comprehensive suite of unit tests. The changes span across different layers of the application, from front-end Vuex actions and UI components to back-end database interactions and REST API endpoints, ensuring that core functionalities are thoroughly validated. Minor code adjustments were also made to facilitate testing and improve code resilience. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request significantly improves the Jest unit test coverage for ZAP, which is a valuable contribution. The changes include adding new test files for actions, queries, and UI components, as well as minor code modifications to facilitate testing by exporting functions. My review focuses on the quality and structure of the new tests. I've identified some areas for improvement, such as removing duplicated tests, strengthening weak assertions, and refactoring overly complex test cases to enhance maintainability and readability. Overall, this is a solid step towards a more robust and well-tested codebase.
JIRA: WT-668
…f jest and cypress coverage for total analysis and updating sonar qube to use the combination to fix the overall test coverage being reported JIRA: WT-668
…coverage goes below 80% - Adding the zap-combine-reports.js process to the github workflows so that the CI fails everytime the code coverage goes below 80% JIRA: WT-668
JIRA: WT-668
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with comments. Improve ZAP over 80% is huge achievement!
| - run: npm run report | ||
| - run: npm run version-stamp | ||
| - run: npm rebuild canvas --update-binary | ||
| - run: npm rebuild libxmljs --update-binary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we are adding these new commands here from line 150-154
| } | ||
| ] | ||
| }) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are all tests in this file just dummy test filled with non-real data for boosting coverage? We do not need to care about what actually is going on in the test function right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain how doing this helps make SonarQube believe the coverage is improved?
| function checkCoverageThresholds(coverageSummaryPath) { | ||
| if (!fsExtra.existsSync(coverageSummaryPath)) { | ||
| console.error('❌ Coverage summary not found') | ||
| process.exit(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When is this function called? Will exit here break any process for the user or Sonarqube unexpectedly?
| 'SELECT CLUSTER_REF FROM FEATURE LIMIT 1' | ||
| ) | ||
| expect(featureRow).toBeDefined() | ||
| const clusterId = featureRow.CLUSTER_REF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onOffCluster.clusterId
Could use this clusterId initiated in a previous test and test if it returns non-empty feature list
|
|
||
| test('checkIfConformanceDataExist returns a boolean', async () => { | ||
| const result = await queryFeature.checkIfConformanceDataExist(db) | ||
| expect(typeof result).toBe('boolean') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should return true in current ZAP database as we have conformance data defined.
see if expect(result).toBeTruthy() can pass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it pass, add it there
| esModules: true | ||
| }) | ||
| .before('vue-loader') | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why we add istanbul dependency here? What does it do?
| json: jest.fn() | ||
| } | ||
| const handler = | ||
| require('../src-electron/rest/user-data').httpPostDuplicateEndpointType( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we import require('../src-electron/rest/user-data') at head of the file? I've seen lots of repetitive imports in the middle of the file
| const resAttr = { status: jest.fn().mockReturnThis(), json: jest.fn() } | ||
| const handlerAttr = require('../src-electron/rest/user-data') | ||
| .post.find((e) => e.uri === restApi.uri.attributeUpdate) | ||
| .callback(db) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
| const resCmd = { status: jest.fn().mockReturnThis(), json: jest.fn() } | ||
| const handlerCmd = require('../src-electron/rest/user-data') | ||
| .post.find((e) => e.uri === restApi.uri.commandUpdate) | ||
| .callback(db) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this structure of handlerCmd being defined many times. Maybe turn it into a helper function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function testAPIHandlers(apiKey, req, expectedResponseObj)
This could make adding new handlers in the future into this test much easier.
| }) | ||
| expect(wrapper.html().length).toBeGreaterThan(10) | ||
| }, | ||
| timeout.short() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Saw lots of repetitive code here for all vue pages. Not sure if it's good to write a helper function and call them here too. Just like helpers for the API handlers I commented before.
JIRA: WT-668