Skip to content

Close more old stage1 issues #13960

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
merged 2 commits into from
Dec 22, 2022
Merged

Close more old stage1 issues #13960

merged 2 commits into from
Dec 22, 2022

Conversation

wooster0
Copy link
Contributor

How can I help closing old stage1 issues?

The current policy is that before an old stage1 issue can be closed, test coverage for it has to be added.

  1. Go to https://github.com/ziglang/zig/issues?q=is%3Aopen+is%3Aissue+label%3Astage1

  2. Close your eyes and click on an issue

  3. Determine whether the code in the issue is supposed to compile-error or simply pass.

  • If the code from the issue is supposed to just pass, maybe clean it up a bit and put the code in test/behavior/bugs/github_issue_number.zig and then reference that file in test/behavior.zig at the right spot (there's an order). Use zig test x.zig to check locally if it does actually pass.
    Example and its reference in test/behavior.zig
  • If it's supposed to compile-error, create an explicitly-named file for how the code is supposed to compile-error in test/cases/compile_errors/. You can check the code locally with zig test x.zig to check what errors you get and copy-paste them into the file at the end as comments like in the other compile error case files.
    Example
  1. In your commits' description you can list all issues you provided test coverage for like Closes #1 each on a separate line, like in the commits of this PR.

@wooster0 wooster0 force-pushed the stage1cruft branch 2 times, most recently from a85823b to ba1ce49 Compare December 15, 2022 20:41
@wooster0 wooster0 force-pushed the stage1cruft branch 6 times, most recently from 13855e0 to e08cd36 Compare December 16, 2022 13:12
@wooster0
Copy link
Contributor Author

wooster0 commented Dec 17, 2022

The two failures only in linux-debug look kind of sporadic.

Edit: and just after I sent this comment the linux-debug CIs are magically re-running? Same failure though so I'm going to rebase.

@wooster0
Copy link
Contributor Author

All 118 tests passed.
thread 776410 panic: integer overflow
/home/ci/actions-runner11/_work/zig/zig/src/type.zig:3579:55: 0x36e66c7 in intAbiAlignment (zig)
            std.math.ceilPowerOfTwoPromote(u16, (bits + 7) / 8),
                                                      ^

These two CI failures are a mystery to me. If I just zig test (no optimizations and I'm on x86_64-linux so it'll be equal to x86_64-linux-debug) every new file in test/behavior/bugs/ myself, it all passes. I also checked the CBE with -ofmt=c.

@Vexu
Copy link
Member

Vexu commented Dec 17, 2022

These two CI failures are a mystery to me. If I just zig test (no optimizations and I'm on x86_64-linux so it'll be equal to x86_64-linux-debug) every new file in test/behavior/bugs/ myself, it all passes. I also checked the CBE with -ofmt=c.

The failure depends on whether your compiler was built with safety checks and is caused by bugs/12116.zig

@wooster0 wooster0 force-pushed the stage1cruft branch 2 times, most recently from 5bf200a to 45cd756 Compare December 18, 2022 12:40
@wooster0 wooster0 requested a review from Vexu December 18, 2022 17:40
if (builtin.zig_backend == .stage2_x86) return error.SkipZigTest; // TODO
if (builtin.zig_backend == .stage2_aarch64) return error.SkipZigTest; // TODO
if (builtin.zig_backend == .stage2_arm) return error.SkipZigTest; // TODO
_ = wuffs_base__make_io_buffer(undefined, undefined);
Copy link
Member

Choose a reason for hiding this comment

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

need to actually test that the code runs correctly; it's not enough to just check that the compiler doesn't crash.

Copy link
Contributor Author

@wooster0 wooster0 Dec 21, 2022

Choose a reason for hiding this comment

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

Alright, I tried to come up with some things I could do there. You can find it in my latest force-push.

@andrewrk
Copy link
Member

Note that test/behavior/bugs/*.zig is also kind of a chore list for me. Ideally these would be properly categorized like this: #13997

It's a slight improvement to get from github issues to test/behavior/bugs/*.zig, so I still appreciate the work you're doing here, but it would be even more helpful if the behavior was categorized.

@wooster0
Copy link
Contributor Author

I fully agree on that and was going to bring up this kind of dependence on GitHub because I don't think we should really reference GitHub issues in our codebase. The issues only exist on Microsoft's GitHub and I think it's best to stay flexible in terms of the code hosting platform. Maybe someday Zig uses a different forge like sourcehut or something.

@wooster0
Copy link
Contributor Author

I was considering removing the newly added issue files but as you said it's still a slight improvement over what we have currently so.

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

Behavior tests need to verify that the logic ran correctly, not only verify that the compiler did not crash.

@wooster0
Copy link
Contributor Author

Right, that makes sense. Something like this?

@andrewrk andrewrk merged commit f211c15 into ziglang:master Dec 22, 2022
@andrewrk
Copy link
Member

Wonderful, thank you!

TUSF pushed a commit to TUSF/zig that referenced this pull request May 9, 2024
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