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

chore: reenable all tests and update snapshots #2370

Merged
merged 2 commits into from
Jul 11, 2023
Merged

Conversation

daKmoR
Copy link
Member

@daKmoR daKmoR commented Jul 7, 2023

What I did

@changeset-bot
Copy link

changeset-bot bot commented Jul 7, 2023

⚠️ No Changeset found

Latest commit: 4c331b2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@thepassle
Copy link
Member

This PR seems to mostly fix the failing tests on master, but now the tests that were supposed to be fixed by #2366 (by @tmsns ) are broken again, seemingly without any apparent reason. @tmsns would you mind taking a look at this?

image

@koddsson
Copy link
Contributor

If we don't come up with a quick solution to this, we can look into disabling these tests again temporarily in order to get all the tests running again on master.

@tmsns
Copy link
Contributor

tmsns commented Jul 10, 2023

@thepassle here is a proposal to fix the "random" timeouts. Locally it works. Let's see if it works correctly in a pipeline.

@koddsson
Copy link
Contributor

@daKmoR; wanna pull master into this branch after we merged #2371? That might have fixed the failing tests 😄

return loadScript('./entrypoint-b.js', 'module', []);
}, function () {
return loadScript('./entrypoint-a.js', 'module', []);
Copy link
Member Author

Choose a reason for hiding this comment

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

hmmm I'm just wondering is the order here maybe wrong?

as it's now entrypoint-b and then entrypoint-a 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I dont think the order matters here?

@koddsson
Copy link
Contributor

This is looking green except for the windows tests hanging, which I believe is a separate issue.

@tmsns
Copy link
Contributor

tmsns commented Jul 10, 2023

Took a first look at the Windows pipeline. Seemed to be hanging once wireit starts the tsc —build —pretty command in the new mocks project. Not sure whether wireit can’t start it or typescript can’t complete it.

Copy link
Contributor

@koddsson koddsson left a comment

Choose a reason for hiding this comment

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

This looks pretty good to me! I'm not sure on the snapshot tests but I'm not sure if the order matters.

@@ -4,6 +4,7 @@ on: pull_request

jobs:
verify-windows:
timeout-minutes: 30
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that we have a timeout now. I'd argue that we can probably make it even lower but maybe in a future PR.

@thepassle
Copy link
Member

So is this an issue with wireit? It seems unlikely that its an issue with TSC, right? Should we disable wireit for now, and just run TSC directly? What do you think @daKmoR ?

image

@thepassle
Copy link
Member

Not really sure why, but I'm still seeing wireit running in the pipeline, so it's still hanging on Verify Windows

image

@tmsns
Copy link
Contributor

tmsns commented Jul 11, 2023

@web/mocks itself is also using wireit 🙈. It’s not only on top level.

"types": "wireit",

@koddsson
Copy link
Contributor

koddsson commented Jul 11, 2023

Now this is happening without wireit! 🤔

Run npm run build
  npm run build
  shell: C:\Program Files\PowerShell\7\pwsh.EXE -command ". '{0}'"
  env:
    WIREIT_CACHE: github
    ACTIONS_CACHE_URL: https://artifactcache.actions.githubusercontent.com/ccnqSCum0KFcxsiFoFI7NcNXpjM[3](https://github.com/modernweb-dev/web/actions/runs/5523695137/jobs/10074963603?pr=2370#step:8:3)eDUNQ90mJ1DwqHCbmkBIOA/
    ACTIONS_RUNTIME_TOKEN: ***

> build
> rimraf --glob packages/*/tsconfig.tsbuildinfo && tsc --build && npm run types


> types
> cd packages/mocks && npm run types


> @web/[email protected] types
> tsc --build --pretty

Terminate batch job (Y/N)? 
Error: The operation was canceled.

@koddsson
Copy link
Contributor

Maybe we can enable the diagnostics flag to get a better idea of what's going on?

I googled around and someone talked about this happening on machines with lower memory and I did discover that the Windows machines have 7GB memory while macOS has 14GB which might explain why it would only hang on Windows machines.

https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners

@daKmoR
Copy link
Member Author

daKmoR commented Jul 11, 2023

ok, I now specifically NOT do the types action on windows (there is no need to do it anyways)

still there is one failing test... but I will leave this for another PR

@daKmoR daKmoR merged commit dd5e0e5 into master Jul 11, 2023
4 of 5 checks passed
@daKmoR daKmoR deleted the chore/renableTests branch July 11, 2023 21:27
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.

4 participants