[feature request] RFC: Unit tests

[kfern]

Is your feature request related to a problem? Please describe.

Unit testing is a "legacy todo".

If we like to have it we must do a big refactor because the components are not isolated. They are using the store inside it.

For example this function https://github.com/ONEARMY/community-platform/blob/1535808b3951c88b2a86d06d3fb3c5ef14c2fdbd/src/pages/Maps/Content/Controls/GroupingFilterDesktop.tsx#L75 do calls to mapsStore.getPinsNumberByFilterType before render for each option. PS: This behavior may be related with #1257

Describe the solution you'd like I'm going to use the Maps page as a proof of concept for a new unit tested version.

Because a line of code is better than 1000 words, I have prepared a temporary repository at https://github.com/kfern/wip-maps-page. I'm not solving every use cases but It can be a starting point.

I'm using checkboxs in this poc because @khanacademy/react-multi-select looks like it's dead. The last commit was on May 28, 2019 and does not support typescript.

Feel free to close this issue if it's not a good idea.

[thisislawatts]

Thanks for raising this @kfern 🙇🏻 I will take a look at this over the next weekend.

Very interested in improving the test coverage on this project!

[thisislawatts]

Thanks for the patience @kfern, I got some time this morning to get the demo repo up and running. 🤩

I think this is a really great step forward, and I appreciate you putting into code some of the issues I have experienced whilst working with the stores over the last few months.

I am curious why you opted to spin up a standalone repo rather than working within a branch of the community-platform codebase?

Given the performance issues some users are experiencing on the map page I do think we need to take another look at the current implementation.

How can I help you to get this raised as a PR against the primary repo?

[kfern]

@thisislawatts I'm glad you like it :-D

I created a new repository because we can go further with integration and e2e testing using GitHub actions, and I will need a "lab" to test.

I am using updated versions of leaflet related components and they are not compatible with the actual dependencies. Could you align both? I could downgrade if you prefer.

I also encounter problems passing lint

[thisislawatts]

@kfern what do you think is the smallest step we could take to move the project towards the proposal you have outlined?

I had missed the GitHub workflows. At the moment, I would prefer to keep Circle CI as the primary location for verifying the project. We should be able to make this change at a later date but I do not think it should coupled with this refactor.

[kfern]

@thisislawatts. I think the first step would be to make a list of the use cases that need to be solved in order to use it in production.

We don´t need Github Actions by now.

[thisislawatts]

Could you clarify what you mean by use cases?

I was thinking about a way to perform an incremental refactor allowing us to release in phases rather than a single PR that switches from the existing implementation to an entirely new versions.

For example, as you've already mentioned:

1. Upgrade leaflet
2. Replace deprecated khan academy component

[kfern]

Sorry, I was talking about the full refactoring process.

Do you plan to update the components to the latest versions?

The easiest solution would be to use the oldest version. I can downgrade the RFC.

What do you think?

[thisislawatts]

I would prefer to see these components upgraded. If we're in the area we should move things forward. Would you like to raise a PR that makes that update?

Edited to include this uttering from Kent Beck, which echos in my head:

for each desired change, make the change easy (warning: this may be hard), then make the easy change

[kfern]

Hi @thisislawatts

I have looked at this possibility but I think it's better to update dependencies after code refactoring because we are going to break things when we upgrade the components.

Also, we can't add unit tests to the actual implementation because the components involved have multiple side effects.

The "smaller steps" may be:

• PR aligned with current dependencies. I can do it.
• Create a "hidden" URL that displays the new implementation using the real database. Could you do this?
• Implement all the use cases that need to be solved to use it in production. This task could be shared and it is an opportunity to "rethink" the functionality of the page.
• Replace the current map component. At this point we should be confident with the new implementation.
• Update related dependencies.

[thisislawatts]

That sounds good to me @kfern, if you can push your work into a branch on a fork of this repo we can collaborate on this 👍🏻

I went to look at the existing e2e for this page to see what use cases are documented, not much guidance to be found 😁 https://github.com/ONEARMY/community-platform/blob/master/packages/cypress/src/integration/map.spec.ts#L5-L7

We have our next monthly dev call on Monday 7th Feb. Would this timeline work for you to have something we could share with the team and get everyone's feedback on?

[kfern]

I guess we can have something. I'm very busy at this moment, but I can find a little time each work day.

Right now, the most important step is to have the use cases. This task can be done with the team.

[thisislawatts]

Sounds good, no urgency on this from my side @kfern, so happy to work at the pace which works for you.

I will look at gathering the use cases over the next week or so.

[kfern]

Hi @thisislawatts. I can't push it. I have some @typescript-eslint/no-empty-function errors. I'm using empty functions because the MapController injects props to the Map view component (Dependency inyection).

This rule was removed from eslint-config-standard-with-typescript as it is a common use case in React. https://github.com/standard/eslint-config-standard-with-typescript/issues/248

Could you disable it?

Another question: Do you know how I can see the image src/assets/icons/map-workspace.svg in Storybook? http://localhost:6006/assets/icons/map-workspace.svg shows a 404 error.

[thisislawatts]

@kfern, thanks for flagging this rule conflict, any reason not to do one of the following?

1. Disable this rule in your branch
2. Disable the rule on a file by file or directory basis within your branch

I will take a look into the separate asset issue you mentioned :)

[kfern]

@thisislawatts: PR aligned with current dependencies done!

[chrismclarke]

Sounds super interesting, thanks for all your input on this @kfern . I just checked out your branch and really like being able to see all the individual components in storybook and run all the individual tests.

My one thought is whether there would be any sense in keeping the store but providing a mock implementation that generates data (in the same way your test utils do)? Pretty much all feature modules use stores, so I'd be a bit worried about how best to maintain tests in cases where the component includes data from a user that would be populated via the store. I wouldn't want to write individual test utilities to generate that data for every module, and so assuming there is one common set of test utils per module would wonder if just easier to keep references from one to another via stores instead of individual imports. But of course, at this point do we still have a unit test or is it more of an integration test...

It seems that the jest docs point to an (old) article about ways to unit test with mobx here: https://jestjs.io/docs/testing-frameworks, although I'm definitely no expert on the matter so a genuinely open question in case anyone else has strong opinions.

Create a "hidden" URL that displays the new implementation using the real database. Could you do this?

Yeah, we should definitely find a way to make it easier to get 'experimental' modules accessible on the dev site. I'll try scaffold out a PR to help with this. (update - opened #1394)

I have some @typescript-eslint/no-empty-function errors. I'm using empty functions because the MapController injects props to the Map view component (Dependency inyection). This rule was removed from eslint-config-standard-with-typescript as it is a common use case in React. standard/eslint-config-standard-with-typescript#248 Could you disable it?

Makes sense, will create a PR for this also (update - opened #1395)

[chrismclarke]

A couple screenshots from your repo just for future ref:

[kfern]

Hi @chrismclarke

It seems that the jest docs point to an (old) article about ways to unit test with mobx here: https://jestjs.io/docs/testing-frameworks, ...

You can test mobx but you can't add unit tests to your implementation because you need to prepare a lot of things before.

I think you will have a similar problem if you try to create a story for the current map page implementation.

My one thought is whether there would be any sense in keeping the store but providing a mock implementation that generates data (in the same way your test utils do)?

Following my proposal we are going to have 2 components:

• MockDatabaseProvider: Generated Data Factory for unit tests. We should have a single component and factory functions by module / database collection.
• DatabaseProvider: Database interface. We can use Firestore query streams or get

What do you think?

[chrismclarke]

Oh, a MockDatabaseProvider makes a lot of sense to me. The main function of the stores typically are to a) get/stream db data and b) update data on user interaction (e.g. filter, go to active etc.), so I like the idea to keep the store use for integration/e2e and have a cleaner direct line to data for unit tests (which wouldn't need to.

The current database provider is already an abstract wrapper around DB methods anyway (e.g. current app uses firestore db, firebase realtime db, and dexie db all from the same provider), so should be a pretty smooth process to also add the mock. The main challenge I can think of is how best to manage folder structures, but I think that would go a bit beyond this PR so probably best to focus on getting things working here first and worry about the rest later.

So if I understand rough order of priorities building on your previous list above we have:

Stage 1

• Get refactored current code working with live data (as experimental page, requires #1394)
• Test functionality, minor fixes
• Discuss/demo on next dev call to identify list of any further tests required, future changes to how the map should work, general feedback etc.

Stage 2

• Build out any additional tests required
• Merge into main branch to replace old map implementation
• Create issues for any future map features we want to integrate

Stage 3

• Generalise DB methods to make it easier for other modules to stub out unit tests in a similar way
• Document and start to build out unit tests for other modules

Does that sound about right?

[kfern]

@chrismclarke. SGTM