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

tests: organize lang spec and generic related tests #1175

Open
wants to merge 16 commits into
base: devel
Choose a base branch
from

Conversation

saem
Copy link
Collaborator

@saem saem commented Feb 8, 2024

Summary

  • what changed and how?
  • why are we changing it?

Details

  • info that couldn't fit into the summary
  • relevant details; tricky parts
  • anything else

TODO

  • shift focus towards generics and keep going until tired

Notes for Reviewers

  • test re-org is messy stuff, no real narrative

saem added 8 commits February 8, 2024 12:22
that stuff is after we have a module system, and includes are more
template related.
it made no sense to have them separately
along with a few minor clean-ups in `lang`
the test still needs further clarification and can likely be reduced
This is a dumb 'feature', it doesn't make sense and should never pass.
It clearly demonstrates a bug where we equate `any` with `untyped`.

Mark the test as a `knownIssue` so we know when it starts failing again.
@saem saem added spec Specification tests edit or formal specification required. test Add or improve tests labels Feb 8, 2024
@saem
Copy link
Collaborator Author

saem commented Feb 10, 2024

I'm re-running the tests, I can't replicate the failure locally.

@zerbina
Copy link
Collaborator

zerbina commented Feb 16, 2024

Huh, that's strange. tjsonutils.nim consistently fails for the Windows and MacOS CIs, but not on the Linux CI. The only reason for this I can think of is that moving around, removing, or adding new tests changed the make-up of the megatest in a way that caused a latent bug to surface.

@saem
Copy link
Collaborator Author

saem commented Feb 16, 2024

Huh, that's strange. tjsonutils.nim consistently fails for the Windows and MacOS CIs, but not on the Linux CI. The only reason for this I can think of is that moving around, removing, or adding new tests changed the make-up of the megatest in a way that caused a latent bug surfacing.

Yup, that's what I'm thinking too. Locally, I get js failures for tjsonutils on line 119. My best guess is it has something to do with the define nimPreviewJsonutilsHoleyEnum. It's specified in tests/nim.cfg and jsonutils.toJson conditionally compiles on it within generic code. I wonder if it's being instantiated one way and then differently at some other time.

I'm not quite sure why fromJson is failing in CI on line 286, the only thing they have in common is isNamedTuple but it's been around for a while.

Hmm, given the common denominator is isNamedTuple right now, then I'll start by investigating that I suppose.

@saem
Copy link
Collaborator Author

saem commented Feb 17, 2024

Just a small update:

In terms of test ordering and population, the only difference is linux includes (tests/misc/tsizeof4.nim), while macos m1 does not -- it's disabled on arm64.

So I get local failures if I run tjsonutils for the js target, but no issues when doing a full run across all targets with megatest. I'm going to see if I can sort out the local failure issue and why it's not failing when run in a megatest context. Hopefully that'll help shed some light on things.

@saem
Copy link
Collaborator Author

saem commented Feb 18, 2024

i haven't quite figured out how, but there seems to be a js codegen issue for tjsonutils:119. the passed in parameter seems to have field positions swapped so the values are assigned incorrectly.

Here is a snippet from the generated js:

var Data5 = {jsonNodeMode: 1, enumMode: 0};
var Data6 = {jsonNodeMode: 2, enumMode: 0};
var Data7 = {enumMode: 2, jsonNodeMode: 0};

This corresponds to initializations of ToJsonOptions, the third item has the fields in the correct order. The first two the fields are in the incorrect order (not really a big deal), but the values are assigned incorrectly, jsonNodeMode should be 0 in both cases, while enumMode should have 1|2 assigned.

Just gotta track down where this is happening in jsgen.

Update: after some further debugging the issue might be earlier, in mir or transf.

@saem
Copy link
Collaborator Author

saem commented Feb 18, 2024

With the above issue fixed in the following PR, thanks to @zerbina, going to see if that also resolves the issue encountered in CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec Specification tests edit or formal specification required. test Add or improve tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants