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

Automatic testing of comments #1538

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Conversation

Julow
Copy link
Collaborator

@Julow Julow commented Nov 6, 2020

Related issue: #1503

Finding a test case for every call sites is not possible: some calls are unreachable and others are reachable in specific cases that are very hard to guess due to side effects and unspecified internals.

This PR tries to fix this in an automated way. For each tokens in a source file, a comment is inserted (then removed), formatting is called and the diff is printed.
I added a sample containing a big part of the language, more is still needed.

This finds:

  • a lot of inserted newlines (sometimes 2)
  • moved comments that can be considered bugs
  • dropped comments

There is also a lot of false-positive:

  • wrapping because of the margin on long lines (the sample file could be improved)
  • moved comments that are definitely not bugs (eg. M.(* comment *)N)

The gained coverage is not huge (116/154 covered vs 91 on master) and we still cannot say which are unreachable.

The diff algorithm is from https://github.com/craigfe/diff. Other released libraries have incompatible dependencies.

@gpetiot
Copy link
Collaborator

gpetiot commented Oct 11, 2023

@Julow Is this still useful? I believe we have now enough tests on comments. What should happen to this PR?

@Julow
Copy link
Collaborator Author

Julow commented Oct 11, 2023

I don't think we have enough test on comments.
Before closing this, it would be nice to rebase and run this again to have an idea of the current state, which has probably improved.

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.

3 participants