-
Notifications
You must be signed in to change notification settings - Fork 38
Add Unit Tests for Application and New CI Workflow for Vitest Coverage #1379
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: develop
Are you sure you want to change the base?
Conversation
|
Hi @EnsiyehE |
|
Use Run test server using develop.opencast.org as backend: Specify a different backend like stable.opencast.org: It may take a few seconds for the interface to spin up. |
Arnei
left a comment
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.
Thanks for introducing unit tests to the admin interface. The overall approach looks good I think, but I still have a couple comments and questions for you (Some of which likely come across as terse. I assure you I am not trying to be rude though).
Running npm test produces a couple warnings and errors in the console. Are they expected? If so, would it be preferable to handle them in some way so as not to confuse others?
Please adapt the gitignore file to include files generated by the tests and coverage, e.g. /coverage.
The npm run build command is failing. Looks like eslint expects different syntax. Could you fix that?
Oops! Something went wrong! :(
ESLint: 9.32.0
A config object is using the "env" key, which is not supported in flat config system.
Flat config uses "languageOptions.globals" to define global variables for your files.
Please see the following page for information on how to convert your config object into the correct format:
https://eslint.org/docs/latest/use/configure/migration-guide#configuring-language-options
If you're not using "env" directly (it may be coming from a plugin), please see the following:
https://eslint.org/docs/latest/use/configure/migration-guide#using-eslintrc-configs-in-flat-config
| @@ -0,0 +1,38 @@ | |||
| name: Vitest Coverage | |||
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 should we add code coverage reports to our releases? I feel like it will just unnecessarily bloat our releases with files that no one will ever look at.
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.
It was my idea to give the topic of testing a bit more visibility. If we only create them as artifacts of e.g. each action run, I think nobody will ever look at them. This was just a first idea. I think there are other options as well. Maybe just publishing them using github.io might also be a possibility. Other ideas are also welcome.
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.
Adding test coverage to the releases is likely to get ignored, especially since we're aiming to automate away releases of the admin interface to Opencast as much as possible.
Another idea would be to add the coverage value to the Opencast release notes, somehow. Although I am not sure we want to proudly announce 5% code coverage in our release notes, so maybe not ^^'.
May be better if we could automatically post to github discussion, or even the matrix channel? No idea how to do that though.
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.
With all due respect, what I see here already represents nearly 20% of the entire application—a considerable portion of a clearly large codebase. I believe this level of coverage provides a solid starting point that could even be built on by myself.
This is simply my opinion, shared with the intention of contributing constructively.
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 did not mean to belittle your work, this is definitely a large step up from our previous situation. Still, whether it is 5% or 20%, neither of these numbers is nearly large enough to inspire confidence in our adopters (and especially new adopters). Instead, I fear that it would have quite the opposite effect. Therefore, I think we should consider carefully how we want to draw attention to this.
|
I have fixed whatever we discussed according to your solutions , but still build command would give me error regarding something related to Linux , because my machine now is windows as an operating system , i am not sure how i can fix it , i would appreciate your hints and code review again Arnei , thank you ! |
package.json
Outdated
| "eslint-plugin-vitest": "^0.5.4", | ||
| "jsdom": "^26.1.0", | ||
| "msw": "^1.3.5", | ||
| "prettier": "^3.6.2", |
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.
We can certainly discuss if we want to introduce an opinionated code formatter to the project, but let's do so in an issue separate from this PR. As such, please remove prettier from this PR and undo the automatic formatting it applied to previously existing files.
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.
You're absolutely right, and I’ll work on removing Prettier from this PR and undoing the formatting changes as requested.
Apologies for the confusion — I think the formatting on my end may have used 4 tabs, which might be getting interpreted as spaces by others. Outside of using a formatter, I don’t really have another reliable way to keep indentation consistent, but I’ll do my best to adjust it manually this time. Thanks for the clarification!
| > | ||
| >; | ||
|
|
||
| export const rootReducer = reducers; |
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 not directly export reducers? This feels needlessly convoluted.
| import { MemoryRouter } from "react-router"; | ||
| import { render } from "@testing-library/react"; | ||
|
|
||
| export function renderWithStore( |
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.
The renderWithStore from setUpStore.tsx now support routing, right? Why not use it here then?
It now builds fine, at least on my machine ^^'. Whatever you did seemed to help, thanks for that! |
|
This pull request has conflicts ☹ |
|
This pull request has conflicts ☹ |
|
@EnsiyehE are you planning to address the outstanding requested changes? A brief overview:
|
This PR introduces the following enhancements to the application:
Unit Tests
Added new unit tests, including coverage for some components, thunks and events inside the application.
New CI Workflow for Vitest Coverage
Created a new GitHub Actions workflow to run vitest with coverage reporting.
The CI job is configured to run automatically when a new version is released.
Coverage settings are added in package.json to keep it maintainable and version-aware.
Review Request:
Please review the added unit tests and CI workflow. I'd appreciate your feedback on:
The test logic and structure
The setup of the CI job and coverage reporting
Any improvements or missing cases you notice
Thanks for your time and input!