Improve the robustness of the test suite

[mrchrisadams]

The tests we have for checking in put and output in our models calculate figures to high levels of precision, that use numbers that themselves are long strings of numbers, where it's not obvious where they come from. Most of the time, we check if a number is exactly matching the number we have given.

This contributes to our tests being brittle and hard to compare to related work in spreadsheets that we use to explain how the different models work.

For this issue we want to have a documented, easier to understand approach that our tests follow, to make it easier to debug, and easier to explain what is happening at various stages in the models, and how one set of results from one model might be different from another.

[fershad]

Leaving this as a note.

node:test is stable in v20. Addressing this ticket later in the year could also be a chance to remove Jest as dependency from the project.

[mrchrisadams]

Thanks for raising this, fish.

I'm assuming you're referring to this right?

https://nodejs.org/api/test.html

At first glance, the API does look similar to our existing use of Jest - it even seems to support mocking nicely, and so on.

Happy to discuss later in the year - removing dependencies would help, as long it doesn't make support for the platforms we're targeting harder.

We had some earlier problems when targetting node over browsers when developing earlier, and there are a number of new runtimes we probably want to be explicit about supporting / not supporting when making this decision.

[fershad]

Renamed this issue. Currently, the biggest problem we have is our test suite being brittle - in that a change to one input (e.g. global average grid intensity) causes 20-something tests to fail. This requires the expected values for each test to be recalculated and updated so that tests pass again.

In a perfect world, our tests would be robust enough to gracefully handle a change like this without chucking the toys out of the pram.

[mrchrisadams]

hi @fershad - one simple way used on other projects is to agree a sensible range of variation from a given number, and then rather than testing for a value returned to to be EXACTLY a figure, use a range.

Jest has some native support like so, for checking to given number of digits after the decimal point:

test('adding works sanely with decimals', () => {
  expect(0.2 + 0.1).toBe(0.3); // Fails!
});


test('adding works sanely with decimals', () => {
  expect(0.2 + 0.1).toBeCloseTo(0.3, 5); // passes
});

See the Jest docs on toBeCloseTo(number, numDigits) for more.

Another approach is to try using a custom matcher to sanity check that a number is within say… a 10% range either way of what we expect.

An example custom matcher in jest for checking a figure is between two numbers is linked below:

https://gist.github.com/marcelblijleven/70058042eb2054f43c18a24b8516a79e

It's not a huge leap to have a matcher based on the one above that checks a value is within an sensible margin of error, by passing in a single number, then check the returned results is between expected_number - 10% and expected_number + 10%

Worth a try?

https://jestjs.io/docs/expect#tobeclosetonumber-numdigits

[EvanHahn]

Attempted to solve this for the OneByte tests in #151. If that approach looks good, I can make more pull requests.

[fershad]

@EvanHahn I'll check the PR over the weekend when I'm home from travel.

Thank you very much for picking up this issue.

[EvanHahn]

Sounds good. I'd love to help further with this issue. If useful, feel free to message me on the CAT Slack or by emailing [email protected].

[EvanHahn]

@fershad Any chance you could review my PR (#151)? No rush from me, but I'd love to help further.

[fershad]

Sorry @EvanHahn, this one slipped off my radar while traveling. I've just left some comments. 🙏🏽

[fershad]

@EvanHahn I've opened a new PR #156 and started some of the work of tidying up/updating the sustainable web design model tests.

I've update the sustainable-web-design.test.js file for now. Do you want to take a look into the co2.test.js file and see if there are any tests being duplicated, or if any can be moved into the sustainable-web-design.test.js file.

[EvanHahn]

@fershad Yes, will do. I expect to have something by the end of next week.

[EvanHahn]

What is the status of the perDomain, perPage, dirtiestResources, and perParty methods? They do not appear in the documentation so I'm not sure if they need to be tested. If they are private methods, I won't test them. If they're public methods, I will. (And if they're not used, I can remove them completely!)

[EvanHahn]

Relatedly, the SustainableWebDesign class has several methods that are only used internally, and some that are not used at all. Do we expect that people are importing and using the SustainableWebDesign class directly? Or is it safe to remove/refactor some of those functions?

[EvanHahn]

I just opened #157 to clean up the perByte tests. More to come (especially with answers to some of those questions!).

[fershad]

@EvanHahn thanks for this, both of these implementations were before my time but I'll comment on them & pull in others who might be able to add more context.

What is the status of the perDomain, perPage, dirtiestResources, and perParty methods? They do not appear in the documentation so I'm not sure if they need to be tested. If they are private methods, I won't test them. If they're public methods, I will. (And if they're not used, I can remove them completely!)

I believe that these methods are specific to SiteSpeed, and are from the very early days of this library. I think they are still used by SiteSpeed itself, but @soulgalore should be able to provide more context.

Relatedly, the SustainableWebDesign class has several methods that are only used internally, and some that are not used at all. Do we expect that people are importing and using the SustainableWebDesign class directly? Or is it safe to remove/refactor some of those functions?

You're talking about methods like annualEnergyInKwh, annualEmissionsInGrams, and annualSegmentEnergy yeah? I am not aware of anyone using these methods, and they aren't used in the code at all. @drydenwilliams is there any reason you can remember why these were included? Does EcoPing use these methods?