Skip to content

fix(splitter): tests - #253

Merged
agviegas merged 3 commits into
ThatOpen:mainfrom
ShaMan123:fix/ifc-splitter-tests
Sep 11, 2026
Merged

agviegas merged 3 commits into
ThatOpen:mainfrom
ShaMan123:fix/ifc-splitter-tests

Conversation

@ShaMan123

@ShaMan123 ShaMan123 commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Description

Resolve the open topics from #197 :

  • stage ordering
  • failing tests

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following:

  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Follow the Conventional Commits v1.0.0 standard for PR naming (e.g. feat(examples): add hello-world example).
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@ShaMan123 ShaMan123 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

attributed to claude

@agviegas

agviegas commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

The test fixes themselves look good. For the fixture, we'd rather go with the build-step alternative you offered than commit the regenerated school_str.frag: that file is shared (it also feeds the Materials example), and pinning sampleIds is exactly the kind of thing #260 predicts will flake. Generating it at test-setup time keeps the tests honest and the repo lighter. What do you think?

@ShaMan123

ShaMan123 commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

I can do it on test setup, no problem. But it seems an overkill for now, since there is a single test that uses it and it is very quick - so I inlined the conversion as part of the test.

@agviegas

Copy link
Copy Markdown
Contributor

Inlining the conversion works great: with it, extract ifc goes green on our machine too, which means the stale committed fixture was the reason that test was failing on main. One thing: the inline conversion pushes the test past vitest's default 5s timeout on our hardware, so it fails by time, not by logic. Could you add an explicit timeout to the heavy tests (60s is plenty)? With that we'll merge right away.

@ShaMan123

Copy link
Copy Markdown
Contributor Author

Done
Are you planning on a github workflow that runs the tests on PR/main push? I think it is time.

@ShaMan123
ShaMan123 force-pushed the fix/ifc-splitter-tests branch from efba5bc to a97ffff Compare September 10, 2026 15:23
@agviegas agviegas mentioned this pull request Sep 10, 2026
8 tasks
@agviegas
agviegas merged commit ec7a7c7 into ThatOpen:main Sep 11, 2026
@agviegas

Copy link
Copy Markdown
Contributor

Yes: main now has a ci.yml running install, build and the suite on every PR and push. Your timeout fix is what turned it green.

@ShaMan123
ShaMan123 deleted the fix/ifc-splitter-tests branch September 11, 2026 12:40
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