finos / a11y-theme-builder

DesignOps toolchain theme builder for accessibility inclusion using Atomic Design.
Apache License 2.0
37 stars 70 forks source link

[REQUEST] Introduce Testing Framework for Improved Test Structure and correction of pre-defined tests #854

Open Sweetdevil144 opened 1 month ago

Sweetdevil144 commented 1 month ago

Suggestion/Concern

The current test file test.ts could be improved for better readability, maintainability, and effectiveness. Some concerns include:

  1. The use of var for importing axios. It's usually recommended to use const or import for importing modules.
  2. The main function is testing multiple things, making it hard to identify what's being tested and what the expected outcomes are.
  3. Error handling is inconsistent. Some errors are silently ignored with empty catch blocks.
  4. The base URL for API calls is repeated multiple times.
  5. The use of async/await is inconsistent with promise .then().
  6. TypeScript's type checking features are not fully utilized.
  7. Hardcoded base URL and other configurations in tests.
  8. Actual API calls are made in tests, which can be slow and unreliable.

Proposed Solution

  1. Use const or import for importing modules.
  2. Break up the main function into separate tests, each testing one thing.
  3. Improve error handling by at least logging errors.
  4. Store the base URL for API calls in a constant.
  5. Use async/await consistently.
  6. Define interfaces for the responses from API calls.
  7. Use environment variables for configuration.
  8. Consider using a mock server for testing, like Nock.

I'd like to come up with a solution for this Issue by using testing frameworks like mocha, jest or cypress. I'd also update our test cases for better readability and code quality.

Any feedback would be helpful.

Tagging @PaulaPaul and @aaronreed708 for your views on above Proposal.

Sweetdevil144 commented 1 month ago

One more point I'd like to add here. I noticed the following lines in our code/src/test.ts :

    if (false) {
        r = await axios.delete(cfg.baseUrl + "/api/themes");
        console.log("deleteDocs =",r.data);
        myAssert(r.data, "Delete database failed")
    }

By default, this section will never be executed due to presence of false keyword above. If we take one more look, we find an occurrence of let errors = false;. Would this be related anyhow?

aaronreed708 commented 1 month ago

If you could shore up our testing, that would be wonderful! We have certainly been letting that slide. Which tool would you recommend above the others? Would you like to be assigned to the issue?

Sweetdevil144 commented 1 month ago

Would you like to be assigned to the issue?

Yes. Thanks. I'll try to use jest for testing owing to it's flexibility. I can also go with mocha with sinon and chai if we need any mocking to do. I'll wrap it up as soon as possible.

codyzu commented 1 month ago

Any reason not to use the built in test runner that was made stable in node v20? We would still need a library for mocks, but sometimes jest is overkill for newer projects. Just an idea. Thanks for your amazing work on this 🙌

codyzu commented 1 month ago

FYI: I've seen recent projects us the built-in test runner and ts-sinon for mocks.

Sweetdevil144 commented 1 month ago

@codyzu The built in test runner lacks many features that are commonly used in modern testing, such as mocking, spying, detailed assertion libraries, and test coverage reports. Of course we can use sinon for stubbing as well as for mocking but that would just lead to stacking of multiple packages on top of each other. Jest, on the other hand, is a comprehensive testing framework that provides all of these features out of the box.

codyzu commented 1 month ago

I think you are making some awesome progress here. Thank you so much! 🙏

For reference, here are some of the reasons that folks avoid jest with node.js tests:

Of course, I'm quoting a TSC member who is certainly biased toward node's built-in functionality. I don't want to block your progress here, but just make sure we consider all our options. If you or anyone else (i.e. @aaronreed708 or @evangk6 ) has a strong opinion about using Jest, I will support you! Sorry for being a pain 😄

codyzu commented 1 month ago

I just brainstormed with @PaulaPaul . I'm wondering if this would be a good opportunity to compare some of the latest additions to the built-in test runner vs jest. Would you be willing to do a small comparison? Come up with a test case that does some mocking and nice assertions and see what they look like with the built-in runner vs jest? This could be an excellent opportunity to do a concrete comparison. What do you think?