search

DemocracyClub / WhereDoIVote-Widget

:earth_africa: An embeddable widget that consumes the UK Polling Station API
MIT License
5 stars 4 forks source link

Feature/uncontested elections #651

Closed VirginiaDooley closed 1 year ago

VirginiaDooley commented 1 year ago

Closes https://github.com/DemocracyClub/WhereDoIVote-Widget/issues/645

As noted in the comments below, uncontested responses could not be tested in detail. Since the current staging configuration only works with sandbox data, I've included screenshots for uncontested mock data:

**SS30AA - Number of candidates === available seats

Screenshot 2023-01-03 at 10 42 07 AM

**SS30AB - Number of candidates < available seats

Screenshot 2023-01-13 at 6 48 41 PM

**SS30AC - Zero candidates

Screenshot 2023-01-03 at 10 48 52 AM
github-actions[bot] commented 1 year ago

This PR has been deployed.

VirginiaDooley commented 1 year ago

I'm trying to figure out how to run the correct data set for the corresponding environment. I tried creating a dedicated command to test mock environment similar to the test command here, but that did not change the test results. Advice very welcome.

chris48s commented 1 year ago

I may be misunderstanding your question, but I think I see what you are getting at. I'll attempt to say something of use. You've got 6 commands for starting a dev server:

and

You've got only one test command: npm test. The tests start with REACT_APP_API=sandbox, which does make it look like the integration tests would call the sandbox API if you don't explicitly call mockResponse() but it is a red herring. It is just passing a value in here because the application will crap out here if you don't pass a valid value in for REACT_APP_API this is mostly a good thing when it comes to making sure we build a valid app, but what actually happens under test is that regardless of what value you pass for REACT_APP_API axios is completely mocked out to just return { data: {} } as a baseline using a jest manual mock. Then you can call mockResponse() to register a different mock response for .get for each test that needs one. See:

  1. It leads to a situation where it looks like changing REACT_APP_API=sandbox in https://github.com/DemocracyClub/WhereDoIVote-Widget/blob/b07a892d399f22b22b2328db1611ebb260231d23/package.json#L56 would do something, but it actually does nothing. Tbh, if this was in a file format that allowed comments it would probably have one explaining all this, but you can't put comments in JSON so :shrug:.
  2. You can't rely on the integration tests if you're updating axios because the whole package is mocked under test - you really need to actually run the app locally and make some queries against the live or sandbox API to verify an upgrade to axios is non-breaking.

Does that help?

VirginiaDooley commented 1 year ago

I may be misunderstanding your question, but I think I see what you are getting at. I'll attempt to say something of use. You've got 6 commands for starting a dev server:

and

You've got only one test command: npm test. The tests start with REACT_APP_API=sandbox, which does make it look like the integration tests would call the sandbox API if you don't explicitly call mockResponse() but it is a red herring. It is just passing a value in here because the application will crap out here if you don't pass a valid value in for REACT_APP_API this is mostly a good thing when it comes to making sure we build a valid app, but what actually happens under test is that regardless of what value you pass for REACT_APP_API axios is completely mocked out to just return { data: {} } as a baseline using a jest manual mock. Then you can call mockResponse() to register a different mock response for .get for each test that needs one. See:

  1. It leads to a situation where it looks like changing REACT_APP_API=sandbox in https://github.com/DemocracyClub/WhereDoIVote-Widget/blob/b07a892d399f22b22b2328db1611ebb260231d23/package.json#L56 would do something, but it actually does nothing. Tbh, if this was in a file format that allowed comments it would probably have one explaining all this, but you can't put comments in JSON so 🤷.
  2. You can't rely on the integration tests if you're updating axios because the whole package is mocked under test - you really need to actually run the app locally and make some queries against the live or sandbox API to verify an upgrade to axios is non-breaking.

Does that help?

Yes, that does help, thank you. Re: 2 - I've run the app and verified I'm getting the right data and corresponding text for postcode. I would still feel better if I could test it.

chris48s commented 1 year ago
  1. You can't rely on the integration tests if you're updating axios because the whole package is mocked under test

Re: 2 - I've run the app and verified I'm getting the right data and corresponding text for postcode. I would still feel better if I could test it.

So just to note: the axios version isn't being touched in this PR, but I assume you mean when you were working on https://github.com/DemocracyClub/WhereDoIVote-Widget/pull/649

If you want to solve that problem for axios upgrades, you could switch your mocking approach to something like nock which moves the mocking a layer out to mocking the node HTTP API instead of the HTTP client library. That way you could rely on the tests when upgrading axios because your client library code still runs when you make a request - its just taking to a mock node HTTP API.

I'll leave you to think about whether the mileage you'd get out of that change is worth the churn of migrating.

VirginiaDooley commented 1 year ago

Add Uncontested to WDIV Widget

pmk01 commented 1 year ago

Reading the text in the fewer candidates than seats scenario, you've included text from WCIVF which is not relevant to WDIV: "No votes will be cast, and the candidate below has been automatically declared the winner." Can this sentence be removed?

VirginiaDooley commented 1 year ago

Remaining work on the back end for Uncontested Elections

VirginiaDooley commented 1 year ago

Reading the text in the fewer candidates than seats scenario, you've included text from WCIVF which is not relevant to WDIV: "No votes will be cast, and the candidate below has been automatically declared the winner." Can this sentence be removed?

Thank you for catching that. I've removed that sentence from English and Welsh translations as requested with 4887fc6.

Screenshot 2023-01-13 at 6 48 41 PM