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

Code coverage for cypress testing #2031

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

PRINCESANCHEZ
Copy link
Contributor

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

Prince Sanchez added 2 commits June 20, 2024 11:26
canvas_modules/harness/cypress/support/e2e.js Outdated Show resolved Hide resolved
canvas_modules/harness/package.json Outdated Show resolved Hide resolved
canvas_modules/harness/webpack.config.dev.js Outdated Show resolved Hide resolved
canvas_modules/harness/webpack.config.prod.js Outdated Show resolved Hide resolved
@@ -0,0 +1,24 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why this is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.nycrc file is needed to configure NYC which generates code coverage reports.

Copy link
Member

Choose a reason for hiding this comment

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

Can you give more details on this? For example how does webpack get used and is there any documentation on this?

Copy link
Member

Choose a reason for hiding this comment

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

I removed this file and it doesn't seem to make a different in the result.

canvas_modules/harness/.babelrc Outdated Show resolved Hide resolved
canvas_modules/harness/package.json Outdated Show resolved Hide resolved
@@ -141,4 +141,4 @@ module.exports = {
rules: rules
},
plugins: plugins
};
};
Copy link
Member

Choose a reason for hiding this comment

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

Why are these files changed? Looks like an addition line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the webpack.config.dev.js and webpack.config.prod.js, I reverted them to their original state because I believe I added a comma in the previous commit last Thursday.

Copy link
Member

Choose a reason for hiding this comment

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

These are still showing up as changed though.

@@ -0,0 +1,24 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Can you give more details on this? For example how does webpack get used and is there any documentation on this?

"build": "npx stylelint 'src/**/*.scss' 'assets/**/*.scss' && grunt",
"test": "npx cypress open",
"test": "npx cypress open && npx cypress run",
Copy link
Member

Choose a reason for hiding this comment

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

Why was this changed?

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 initially added the script to see if Cypress would open and run in the terminal simultaneously, but unfortunately, it didn't work as expected. I intended to revert the changes but it seems I have forgot to.

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 not able to comment on your question above, but I did some more research and the .nycrc file configures NYC (Istanbul's CLI) for code coverage, specifying which files to include, exclude, and the reporting format. Webpack is used with Babel to instrument the code via babel-plugin-istanbul before bundling it. Here was the documentation for NYC https://github.com/istanbuljs/nyc, it can also be found on the cypress docs.

"@pmmmwh/react-refresh-webpack-plugin": "0.5.11",
"ajv": "8.12.0",
"autoprefixer": "10.4.19",
"babel-core": "6.26.3",
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did some research on both babel-core and @babel/register and found that they primarily ensure the code runs correctly in environments that don't natively support the latest JavaScript features. Since I was still able to generate code coverage without them, I went ahead and removed these dependencies from the project.

@@ -34,14 +36,19 @@
"@babel/polyfill": "7.12.1",
"@babel/preset-env": "7.24.3",
"@babel/preset-react": "7.24.1",
"@babel/register": "7.24.6",
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed?

@@ -5,8 +5,10 @@
"license": "Apache-2.0",
"scripts": {
"start": "NODE_ENV=development node index.js",
"cypress:run": "cypress open",
Copy link
Member

Choose a reason for hiding this comment

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

Why did we add this? We already run a similar cmd under test.

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 believe this was an extra script, so I went ahead and removed it to eliminate redundancy.

@@ -62,7 +67,10 @@
"grunt-yamllint": "0.3.0",
"html-webpack-plugin": "5.6.0",
"ibm-design-icons": "2.1.4",
"istanbul": "0.4.5",
"istanbul-instrumenter-loader": "3.0.1",
Copy link
Member

Choose a reason for hiding this comment

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

Is that actually used?

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 still able to generate a coverage report without these, so I removed them

@@ -62,7 +67,10 @@
"grunt-yamllint": "0.3.0",
"html-webpack-plugin": "5.6.0",
"ibm-design-icons": "2.1.4",
"istanbul": "0.4.5",
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this module. It says it's no longer maintained and shouldn't be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding your comment on generating a coverage report, I updated the 'coverage' script to: "coverage": "npx cypress run && npx nyc --reporter=html --reporter=text-summary". This way, when you run npm run coverage, it will execute all the tests. Once all 43 tests are finished, you can open coverage/index.html to view the report in the browser.

Copy link
Member

@matthoward366 matthoward366 left a comment

Choose a reason for hiding this comment

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

How does this work? I tried running npm run coverage but that errors out.

If I want to see coverage reports what steps are needed?

@@ -12,13 +12,17 @@ module.exports = defineConfig({
e2e: {
Copy link
Member

Choose a reason for hiding this comment

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

can you add this file to be checked in linting. It should have flagged the new code since we don't use single quotes.

Copy link
Member

@matthoward366 matthoward366 left a comment

Choose a reason for hiding this comment

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

I don't think this is working as expected. This is giving code coverage for the test harness but what we really want is code coverage for common-canvas.

@@ -31,6 +31,7 @@ import "./canvas/utils-cmds";
import "./canvas/verification-cmds";
Copy link
Member

Choose a reason for hiding this comment

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

also add the entire cypress directory. It should have flagged this well in the build.

@@ -0,0 +1,24 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

I removed this file and it doesn't seem to make a different in the result.

Prince Sanchez added 3 commits July 3, 2024 10:58
Copy link

sonarcloud bot commented Jul 31, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants