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: TS base #30

Merged
merged 6 commits into from
Apr 27, 2021
Merged

chore: TS base #30

merged 6 commits into from
Apr 27, 2021

Conversation

vladfrangu
Copy link
Member

No description provided.

@vladfrangu
Copy link
Member Author

CC: @pocesar @B4nan

I forgot to put it in the body of the PR, sorry 😓

Copy link
Member

@B4nan B4nan left a comment

Choose a reason for hiding this comment

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

Looks good, but the CI is failing on windows, I guess some / vs \ issue in paths/jest setup?

@vladfrangu
Copy link
Member Author

Looks good, but the CI is failing on windows, I guess some / vs \ issue in paths/jest setup?

Yeah, that is...weird, considering it worked fine for storage-local... Anywho, fixed

@B4nan
Copy link
Member

B4nan commented Apr 26, 2021

@B4nan
Copy link
Member

B4nan commented Apr 26, 2021

Something is still wrong in there, some tests are executed, and pass, but the build got stuck without any errors:

https://github.com/apify/browser-pool/pull/30/checks?check_run_id=2436401043

Comparing to the linux builds, some tests were not executed in the windows one. I did cancel the build after few minutes as there were no visible updates, but I don't think this is "wait few more minutes and it will pass" problem :D

edit: there is a bit more logs in the windows + node 12 build (but it might be as well unrelated/caused by the manual build cancellation):

https://github.com/apify/browser-pool/pull/30/checks?check_run_id=2436401015

Terminate batch job (Y/N)? 
Entering debug mode. Use h or ? for help. 

At D:\a\_temp\874f3279-d66f-49c8-8524-cfbe0b26c45e.ps1:3 char:5
+ if ((Test-Path -LiteralPath variable:\LASTEXITCODE)) { exit $LASTEXIT …
+     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@vladfrangu
Copy link
Member Author

vladfrangu commented Apr 26, 2021

Well, the tests took 69 seconds on my local PC to execute, the entire time being taken by PASS test/browser-plugins/plugins.test.js (68.029 s). I think this is a case of the CI env being too slow... unless it was fast before this PR?

Edit: Looking at your previous actions logs.. they all took 10+ minutes.. 😬

@B4nan
Copy link
Member

B4nan commented Apr 26, 2021

I see, you are probably right, the tests were slow as hell on windows before (~12 minutes to pass). Will restart the build and lets wait a bit more.

https://github.com/apify/browser-pool/runs/2143312068?check_suite_focus=true

@vladfrangu
Copy link
Member Author

gee, this CI hates me.. Can we get a re-run? 😓

@B4nan
Copy link
Member

B4nan commented Apr 26, 2021

I guess this is ready to be merged, cc @mnmkng

(btw regular reminder that it would be great to change the default to squash merging :])

@mnmkng
Copy link
Member

mnmkng commented Apr 26, 2021

@vladfrangu Before I merge it, could you please bump the version to 1.2.0 and update the changelog? We usually use NEXT instead of the date there, until the version is released. A short mention that we updated to TypeScript would be enough.

@B4nan
Copy link
Member

B4nan commented Apr 27, 2021

The build is failing, and I'd say this is again due to the wrong caching, without lock file, we can't invalidate the NPM cache (this problem is only when re-running the failed builds, as the cache key uses the same run id).

@vladfrangu
Copy link
Member Author

The build is failing, and I'd say this is again due to the wrong caching, without lock file, we can't invalidate the NPM cache (this problem is only when re-running the failed builds, as the cache key uses the same run id).

Triggered CI again, it passed \o/

@B4nan
Copy link
Member

B4nan commented Apr 27, 2021

Yop, because the run id changed. We had the same issue in other repository that used the same caching mechanism.

Will repeat myself, but for me the solution is to add lock files to git and invalidate cache based on it. It will allow proper caching of all node modules, across builds (unless we change some dependencies).

@B4nan B4nan merged commit eda4fa9 into apify:master Apr 27, 2021
@mnmkng
Copy link
Member

mnmkng commented Apr 27, 2021

@B4nan In the SDK check workflow we removed the cache completely and I think it works fine. We can remove it here too. I'm still reluctant to use package lock without dependency pinning.

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