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

- Fixed test compilation #2850

Merged
merged 4 commits into from
Nov 16, 2024
Merged

- Fixed test compilation #2850

merged 4 commits into from
Nov 16, 2024

Conversation

lmartorella
Copy link
Contributor

Description

The test target is showing a lot of Typescript compilation errors.
Some of them are due to missing "include" filter in the karma TS section (and so karma was ending up in trying to compile samples, etc..).
However other TS errors in spec files seems being due to genuine API breaks.

With this PR the compiler phase is now working.

However it seems that there are a lot of dangling errors in automated tests. I cannot see any issue or PR opened for this. Is npm run test / yarn test working on your side?

Chrome Headless 131.0.0.0 (Mac OS 10.15.7): Executed 62 of 171 (12 FAILED) DISCONNECTED (31.872 secs / 0.038 secs)

Thx, L.

@lmartorella
Copy link
Contributor Author

Fixed the DISCONNECTED issue as well, due to infinite loop in the fix collision code. Added a guard to quit the loop in case of probably infinite loop.
In order to keep the guard as simple and performant as possible, the exit check is based on counter, compared to the magnitude of nodes contained.

@adumesny
Copy link
Member

hey thanks for looking at restoring yarn test back, it stopped working when I did some big collision overall and didn't spend the time to fix what could be a valid error or incorrect old behavior (the latter I suspect). then bunch of folders (like React) was added and things don't run.

@@ -15,9 +15,6 @@
"strict": false,
"target": "ES2020"
},
"exclude": [
Copy link
Member

Choose a reason for hiding this comment

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

that may force .spec.ts files to be in the npm build package which we don't want...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are no .spec files in ./src, so this is safe to delete.

while (collide = collide || this.collide(node, area, opt.skip)) { // could collide with more than 1 item... so repeat for each
if (counter++ > this.nodes.length * 2) {
Copy link
Member

Choose a reason for hiding this comment

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

I need to see case where this happens before taking this workaround... will revert it locally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually fixing a broken test, so it is easy to replicate:

Chrome Headless 131.0.0.0: gridstack grid.min/max width/height should set gs-min-w to 2. FAILED
Error: Infinite collide check

Without this safe check, the test suite is simply going in infinite loop causing "DISCONNECTED" on headless or a stuck browser when testing on real browser. In both cases, no results are produced:

Disconnected reconnect failed before timeout of 2000ms (ping timeout)

Copy link
Member

Choose a reason for hiding this comment

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

FYI, I fixed the infinite loop (had NaN for maxW) but left the check in there in case...

@adumesny adumesny merged commit 9041dc3 into gridstack:master Nov 16, 2024
adumesny added a commit to adumesny/gridstack.js that referenced this pull request Nov 17, 2024
fixed the main test file which exposed multiple bugs! :

* GridStack:init(null) didn't set this.opts
* loading exiting layout incorrecly moved items (when just one caused collision)
* update(el, {id: 'newId'}) didn't update the id
* some attr setting optimization

going forward need to get full test coverage to help find issues...
continue gridstack#2850
@adumesny adumesny mentioned this pull request Nov 17, 2024
3 tasks
adumesny added a commit that referenced this pull request Nov 17, 2024
fixed the main test file which exposed multiple bugs! :

* GridStack:init(null) didn't set this.opts
* loading exiting layout incorrecly moved items (when just one caused collision)
* update(el, {id: 'newId'}) didn't update the id
* some attr setting optimization

going forward need to get full test coverage to help find issues...
continue #2850
adumesny added a commit that referenced this pull request Nov 17, 2024
fixed the main test file which exposed multiple bugs! :

* GridStack:init(null) didn't set this.opts
* loading exiting layout incorrecly moved items (when just one caused collision)
* update(el, {id: 'newId'}) didn't update the id
* some attr setting optimization

going forward need to get full test coverage to help find issues...
continue #2850
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