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

[wip] Load type infos correctly #11

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

blizzy78
Copy link
Contributor

@blizzy78 blizzy78 commented May 3, 2023

This PR changes the loading of type infos in GoSourceFile.Incubate such that mutation tests should no longer fail, a bug that had been introduced in #5. With this PR, all tests and mutation tests seem to run fine for me.

I did have to explicitly disable ooze_test.TestOoze_nothing_to_test and ooze_test.TestOoze_with_mutations. These tests do not use regular Go source files on disk, but rather in-memory source "files". Perhaps these should be changed to regular files.
This PR is still a work in progress because of those disabled tests.

I have removed go:build testdata flags from all Go source files in testdata folders. For one, they don't seem to be necessary at all, and two, they prevent this PR from working.

This PR supersedes #10.

@blizzy78
Copy link
Contributor Author

blizzy78 commented May 5, 2023

@gtramontina Do you have thoughts about the disabled tests?

@gtramontina
Copy link
Owner

Thanks for putting in the hard work, @blizzy78! 🙏

@gtramontina Do you have thoughts about the disabled tests?

It seems that the ooze_test.TestOoze_nothing_to_test tests pass, but seeminly for the wrong reason. 🤔

Having the mutation tests running is a testment of the fix, although I'd like to have some level of coverage on internal/ooze/ooze.go. Perhaps, as you said, by using regular on-disk files, or even explore packagetest, as it seems that the package package reads the actual file system.

I'm happy to move forward with these tests skipped for now, but I'll likely come back to them soon.

Comment on lines +44 to +46
if idx < 0 {
return nil
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this ever happen? I'm thinking that if it ever happens, it means that something has actually gone wrong pretty badly, no? I'd consider blowing up with a panic here. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah sounds good to me.

@blizzy78
Copy link
Contributor Author

I didn't know package packagestest existed. This certainly looks interesting. I can try converting the disabled tests to using that.

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.

None yet

2 participants