Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Testing #56

Open
3 tasks
smellyshovel opened this issue Apr 27, 2022 · 14 comments · May be fixed by #111
Open
3 tasks

Testing #56

smellyshovel opened this issue Apr 27, 2022 · 14 comments · May be fixed by #111
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Milestone

Comments

@smellyshovel
Copy link
Collaborator

The most crucial lib's parts should be covered with unit tests. That's the very least requirement till v1.

Write tests for:

  • The lib's public interface
  • Util-methods
  • Helper-methods
@smellyshovel smellyshovel self-assigned this Apr 27, 2022
@smellyshovel smellyshovel added this to the v1 milestone Apr 27, 2022
@smellyshovel smellyshovel removed their assignment Apr 27, 2022
@smellyshovel smellyshovel added good first issue Good for newcomers help wanted Extra attention is needed labels Apr 27, 2022
@wipfli
Copy link
Contributor

wipfli commented May 14, 2022

Thanks for opening this issue. We don't need to cover the whole api right now, but getting a first test in place would be really useful I think.

@smellyshovel smellyshovel linked a pull request Jun 26, 2022 that will close this issue
@smellyshovel
Copy link
Collaborator Author

@wipfli What's the way (in general) of testing things like this plugin? The plugin is dependant on the maplibre gl js, and I need to create a gl js map first to be able to pass it as a mandatory argument to the plugin's constructor, but I get WebGL or canvas-related errors (which is quite expectable TBH, but anyway). When using happy-dom as the environment, I get things like "_canvas.getContext is not a function" and when trying to use jsdom instead, I get Error: Failed to initialize WebGL..

Once again, I'm not nearly an expert in testing, so looking for some general recommendations. Maybe my chosen approach is incorrect by-design?

@smellyshovel
Copy link
Collaborator Author

The other thing.

Testing now seems to be the only remaining thing that holds us from releasing the first major version. Maybe it's worth postponing it up to 1.x? What do you think?

@wipfli
Copy link
Contributor

wipfli commented Jul 1, 2022

We had WebGL errors in the CI of GL JS as well. This was when running on ubuntu. We solved the problem with xvfb, see for example:

https://github.com/maplibre/maplibre-gl-js/blob/ca67214037fb772bb75328c4f72900ed9e740d6e/.github/workflows/test-unit.yml#L52

I think we should have some minimal test coverage for the public API before we release version 1. When we go to this version we make a promise that breaking changes will be reflected by incrementing the major version. But without tests it will be hard to keep the overview. I think new people will find it easier to contribute if CI can check that their contributions don't break the API...

@smellyshovel
Copy link
Collaborator Author

@wipfli Totally agree with you. Let's postpone the v1 release until at least some tests are in place. And thanks for the link, I'll check it out a bit later.

@smellyshovel
Copy link
Collaborator Author

Current status upddate.

I'm unable to test the plugin, because it requeres a map instance. An instance of MapLibre map cannot be created in test environment (tested with happy-dom, jsdom and node) due to errors with both canvas (though this one seems like could be solved) and web-workers, which are both crucial for MapLibre.

@smellyshovel
Copy link
Collaborator Author

@Miguel-Sanches, don't you want to participate in this one? I'm stuck with it (see the comment above), and v1 can't be released until this one is resolved. And the feature you've been working on for so long time can't get into the NPM package because of this blocking one.

@Miguel-Sanches
Copy link
Collaborator

@Miguel-Sanches, don't you want to participate in this one? I'm stuck with it (see the comment above), and v1 can't be released until this one is resolved. And the feature you've been working on for so long time can't get into the NPM package because of this blocking one.

For some reason GitHub didn't notify me of this mention.

I'll be happy to help out with anything needed

@smellyshovel
Copy link
Collaborator Author

@Miguel-Sanches that's wierd. On the other hand, we talked about that in Slack's DMs. Not much changed since then. Everything's in the very same state as it used to be back then.

There's currently a (draft) PR by me where I started to poke around with testing. I think the best idea would be to use Vitest, but I can't get it to work (WebGL- and web-workers-related issues).

Could you please try to check out whether something could be done on that account? Any help is appreciated: research, tests themselves, environment configuration, etc.

Thanks in advance!

@smellyshovel
Copy link
Collaborator Author

Another option would be to use e2e testing. On one side that'd be even better (a more complex approach), on the other side it limits us from testing separate units (like helpers, though they're not the topmost priority at the moment).

@Miguel-Sanches
Copy link
Collaborator

@smellyshovel Will try to have a look at it :)

@wipfli
Copy link
Contributor

wipfli commented Aug 11, 2022

Maybe https://github.com/maplibre/maplibre-gl-geocoder could give some insights. Although tape should not be used anymore, I would rather go with jest for unit tests...

@smellyshovel
Copy link
Collaborator Author

@Miguel-Sanches ⬆️

@Miguel-Sanches
Copy link
Collaborator

Yeap, I'd prefer if we'd go with jest also.

I'll try to take a look at maplibre-gl-geocode during the weekend

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants