Skip to content

fix: add more test coverage for binary expression #2515

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

Merged

Conversation

JesseCodeBones
Copy link
Contributor

Hi Maintainer,
I found the some tests are missed for binary expression, please decide if we need to coverage this part? and please review the below tests are suitable?
image

Regards

  • I've read the contributing guidelines
  • I've added my name and email to the NOTICE file

@JesseCodeBones
Copy link
Contributor Author

@dcodeIO @MaxGraey
Hi Maintainers,
Actually we got a plan that help ASC to improve the test coverage, I understand we prefer the fuzz test as ASC is a compiler, but we also found some issues during the tests adding, so I think it is also benefit if we could review or cover the un-tested code block.
for example: 2403,2428,2379 and 2510.
If your also think it is valuable to continue this work, I will devide the test coverage PR with different functionalities, so it could be easy to review and find if they are meaningful tests.
So can you give an advice? Thank you.
Here are the coverage PRs:
2515,2495,2467,2444,2294.
Regards

@dcodeIO
Copy link
Member

dcodeIO commented Sep 21, 2022

Hey @JesseCodeBones :)

I agree that improving test coverage is a good idea, and appreciate that you are working on this, thanks! There is one somewhat subtle aspect on my mind in addition: There are two builds of the AS compiler, one compiled with tsc to JS and one compiled with asc to AS. The test coverage right now relies on the JS variant to function IIUC, which makes sense since that's what we use in the test suite today. On this background, it would be good if coverage reporting could eventually include Wasm testing as well - just for your information, though, as right now I think the JS variant is sufficient.

I guess we'd start with #2294 and then merge the concrete additions to tests on top, am I seeing this correctly?

@JesseCodeBones
Copy link
Contributor Author

@dcodeIO
Thank you for your rapid reply:)
From my understanding, yes, corrent tests relies on JS variant and the map file will do the source code mapping to the correct ts source file, if the test coverage report is the first step, I think it is a great start.
Regards

@dcodeIO dcodeIO merged commit bf3bf34 into AssemblyScript:main Sep 25, 2022
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

Successfully merging this pull request may close these issues.

2 participants