Improve tests #541
Improve tests #541
Conversation
|
@jackcarlisle looking good so far. keep going. we ❤️ tests! |
|
@jackcarlisle have we lost our CodeCov test coverage report? is it still configured on CodeShip? |
|
@nelsonic Istanbul reporting was lost when we shifted the ui tests to run in karma/phantom (in order to get around the issues they had with browser APIs being required to run). The combination of compiling with babel and webpack and istanbul wasn't working and I had sunk days into trying to get it to work before giving up. You're welcome to try to re-implement istanbul if you like. |
|
@lennym oh, hadn't realised. Is there an alternative way of tracking/reporting the "completeness" of our tests with the karma/phantom approach...? (apologies, I'm haven't used Karma since angular days... thanks!) |
| expect(wrapper.find('FadeImage')).to.have.length(1); | ||
| expect(wrapper.find('h2')).to.have.length(1); | ||
| expect(wrapper.find('h3')).to.have.length(1); | ||
| expect(wrapper.find('h4')).to.have.length(1); |
There was a problem hiding this comment.
These tests feel a bit arbitrary.
- Containing exactly 5
<div/>s isn't really something we care about in the slightest, and seems like it would be very fragile in future code changes, so would just add to maintenance cost without really adding any value in terms of confirming correct behaviour. - If we're going to assert on the headers being present, then I'd probably want to also assert that they contain the text we expect.
|
Having reviewed a good proportion of the code, there's a bit of a recurring theme of "magic number" tests, which are performing assertions on things that are not really relevant or appropriate to the actual functionality we're testing. A particularly striking example is the following:
These don't really provide any value in terms of confirming the correct functionality of components, but will add maintenance overhead when it comes to making updates to components since the tests are incredibly sensitive to change. I'd probably want to try to be more specific about what aspects of components are key to their functionality, which generally will mean making more assertions about components in terms of the classNames and text content, and ensuring the scope of the assertions is limited only to the aspects under tests. |
|
@lennym thanks for the comments! Looking into changing this now! |
|
TODO: refactor tests for |
|
@jackcarlisle can we merge this PR in? (hope your "day off" is going well...) |
|
@nelsonic Yeah it's good to go! |
ready for review
Improve Test Coverage #540