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

Missing tests for Promise combinators #2629

Closed
marjakh opened this issue May 19, 2020 · 4 comments
Closed

Missing tests for Promise combinators #2629

marjakh opened this issue May 19, 2020 · 4 comments
Assignees

Comments

@marjakh
Copy link
Contributor

marjakh commented May 19, 2020

I started looking into which tests exist for Promise.all/allSettled/any/race, and discovered a bunch of missing tests (e.g., a test for Promise.allSettled being there and the corresponding test for Promise.any not being there).

See the (incomplete) table here:
https://docs.google.com/document/d/1lrDTH6UtVj4a_SZcXKBSgknL-wOtDXC5mLQFVcjmgSE/edit?usp=sharing

@rwaldron rwaldron self-assigned this May 19, 2020
@rwaldron
Copy link
Contributor

Thanks for filing—I'm going to prioritize this for tomorrow.

@leobalter
Copy link
Member

@marjakh thanks for sharing this.

Coverage for Test262 has always been a challenge as it sources from each step of the specs.

The Built-in APIs are relatively easier to verify the coverage if compared with the syntax grammar. We mostly have all steps from start to end. Promises being settled add a bit of complexity but it's still ok to be listed from a usage lifetime, from call to return + effects.

All the steps might be expanded in several ways. One case is within abstractions from ECMAScript, e.g. ToLength(x) => ToInteger(x) => ToNumber(x). Another example is expanding input values to different types and values (see the different handlings from ToNumber itself. There are other more subtle expansions as well.

All of this are observable steps to the user of the language and we try to cover tests in the most reasonable ways.

What is complex for me here is the fact this document shows coverage not based on these steps in any order, but actually after each file name. I'm pretty sure naming the files after each step is not helpful as this project historically tried this and failed. We constantly change the steps and that naming convention does not support expansions to abstractions very well. We now have a convention, more implicit than anything, to name files after what is being covered. There is no specific rule that maps these file names the steps being covered.

Add this to the fact the project is maintained by different people with different perspectives and experiences. We can all try to maintain a consistent naming but it's impossible to achieve perfection. Add to the fact some times we need to apply subjective judgement on how much we expand each coverage. It's impossible to explore every expansion and if we ended up comparing methods tested by different people at different times, we will definitely find the subjectivity on each expansion here.

I'm not complaining, it's valid to flag missing coverage for specific parts. What feels odd to me is the document saying a specific test file name is missing in a different folder.

What I'd like to do from this doc is actually map all the contents from each file into a formatted step by step coverage plan for the specs. This way we can even identify other missing points that might not even be yet covered in any of the cited methods. This would also me help to identify the same coverage steps being taking care of with different approaches. Each spec step could be linked to a single file or many files.

I started some mapping to show as an example and I hope I can eventually do some review and complete a full mapping. I'm glad that @rwaldron is taking care of this issue in the meantime.

https://docs.google.com/document/d/1wHW5zfyf2WYzx9WlVjOrrVVu2rldreiDvYrErazcIls/edit

@marjakh
Copy link
Contributor Author

marjakh commented Jun 3, 2020

Hi, sorry for the delay in this reply.

The document is just a way to find out which tests exactly are missing, especially if we have a test for Promise.all and don't have the corresponding test for Promise.any for no reason. It shouldn't be viewed as an end goal to tick all the boxes, it's just a methodology I used to gain understanding on the question "which tests exactly are missing".

In practice, I bumped into this when I did an "optimization" for Promise.any, and no tests failed. I tried to do the same "optimization" for Promise.all, and a test failed. This motivated me to try to get an understanding on whether there are other missing tests like that too. That seems like a situation we could avoid.

@rwaldron
Copy link
Contributor

rwaldron commented Jul 8, 2020

I believe this has been addressed

@rwaldron rwaldron closed this as completed Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants