Repository navigation
Conversation
aJanuary
left a comment
There was a problem hiding this comment.
Amazing work! Will be great to have a test suite to give us some more confidence.
I do have a few concerns about the implementation. Aside from the one about mocking console.log, I think we can reasonably proceed with the PR as-is and schedule the rest for follow-up.
Mocking child components
My preference would be against mocking child components in most cases.
Mocking a child component only proves you passed some value, but it doesn't catch typos in prop names or where the child expects a different shape than the parent provides it. Rendering with a real child exercises the actual contract between them. If a child's contract changes, the mocked version is going to keep returning the old fake output and the parent's test will stay green, even though the app is broken.
It also reduces the amount of maintenance we have to do on the mocks to keep them in sync.
By mocking the child components, we're testing the implementation details - was this sub-component called with this prop. When really we want to be testing user visible behaviour.
I think we would benefit from having the tests more integration-flavoured than hardline isolated tests on the components.
As a blatant appeal to authority, Kent C. Dodds - the creator of the testing library we're using - is in the "integration over mocks" camp https://www.software-engineering-unlocked.com/no-mocks-allowed/
Config duplication
You're mocking the config from scratch in each test. I'm not actually against that per-se. I think people have a tendency to jump too quickly to extracting out any superficial similarities in test code.
But you're also only defining a minimal config needed for that specific test suite, ignoring config keys that are required by the application as a whole. I think this leads to brittle tests. If a config key is already required, then new code in existing components should be able to use it fearlessly. You don't want unrelated tests to break because the code didn't mock the config value because the old code didn't need it. As a general philosophy, I prefer to create a realistically complete environment for the tests to run, rather than hyper-focussing on what the current code happens to need.
And if we are defining an entire config file for each component, we then start to loose the ability to tell what pieces of this config are important for this test and what are there to create the realistic environment.
To that end, the pattern I currently favour is creating a helper to construct the mocks and fakes that configures it with a bunch of reasonable defaults, and then overriding behaviour needed for the specific tests.
Mocking Child Components and Config DuplicationBoth topics are part of the same discussion: what and how we test. Should we write unit tests or integration tests? Kent Dodds is absolutely right when it comes to integration testing. However, he’s wrong—at least in my view—when it comes to test-driven development and unit testing. After 30 years in the industry, I feel confident saying that. I’ve earned a bit of authority on this myself. ;-) As a test-driven developer, I often (admittedly not always) start by writing unit tests and then implement the business logic until my tests pass. When I write unit tests, I’m deliberately not testing user-visible behavior—I’m testing implementation details. In my experience, precise and error-free implementation is more critical to a system’s stability than the part of the software the user ultimately sees. I personally write very few integration tests, and only at the very end of development, when I truly feel the need to test the interaction between my components, services, classes, or functions. Often, this isn’t necessary if the unit tests cover all cases, including edge cases. Admittedly, that’s not the case here: for the MetropolCon 2026, I focused on the components and functions I had to change, and I had to ignore the rest due to time constraints. I use mocks—especially for That’s the software-philosophical side of the discussion. However, the decision on how to proceed ultimately lies with you and James as the project maintainers. If you say you want fewer unit tests and more integration tests, that’s the direction we’ll take. The question is: how do we handle the existing unit tests? They aren’t integration tests, but they are functional unit tests that cover some important implementation details. James and I have agreed that my next steps will include preparing a PR for my changes regarding react-i18n and multilingual support. I’d like to use the current tests for this, as they already exist in nearly identical form in conclar-metropolcon-2026—including multilingual support. In the future, once the MetropolCon changes are integrated into ConClar, we can revisit and refine the tests. What are your thoughts on the next steps? |
|
Thank you so much for your work on this. I've had an initial look and run the tests locally, and it looks like a great start. My partner, Fionna, works as an Automation Architect. We're both new to ViTest, but we'll review over the weekend. I am inclined to favour merging the tests unless we find any major issues with them. We can try to increase test coverage and refine them after that. |
lostcarpark
left a comment
There was a problem hiding this comment.
Fionna and I have reviewed the tests, and we think on the whole they are a great starting point.
We've just highlighted one that we think could use a tweak. The sortname and name values should be different so they will sort differently (we also think adding at least one more item to the people array would be useful).
We'd love to see more test coverage added, but with the change above, we can get this merged and then work on adding additional cover.
|
I think unit tests are very useful for testing isolated functionality, but once we get that in place, an additional set of tests that verify interaction of different components would be very useful. |
lostcarpark
left a comment
There was a problem hiding this comment.
Looks good now. Ready to merge.
|
Oops not quite ready - fixed conflict in dependencies. |
lostcarpark
left a comment
There was a problem hiding this comment.
I've found some issues that I think need investigation, and possible rework.
Could you take a look at the sections I've highlighted?
Thanks!
…t()` for DOM elements
… provide a full state object
This PR introduces unit testing to the project using
Vitest.I used the code already existing in the MetropolCon 2026 version and removed all references to translations with react-i18n (which will be implemented later).
Additionally I made three minor improvements.
Minor Improvements
class→classNameinSwitch.jsxfor consistency with jsx conventionsPeople.jsxto use the configurable search label fromconfig.json.idea/) to.gitignoreUnit Testing
Vitestas the test runner with jsdom environment@testing-library/reactand@testing-library/jest-domfor component testingsrc/setupTests.jsfor global test setupvite.config.jswith test configurationnpm test)The tests are all in separate files, each file follows the naming convention
<component name>.test.js. Dependencies are mocked to ensure the tests run in isolation without external state.