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

Update Typescript and other dev(Dependencies) #440

Merged
merged 1 commit into from Feb 16, 2019
Merged

Update Typescript and other dev(Dependencies) #440

merged 1 commit into from Feb 16, 2019

Conversation

BBosman
Copy link
Contributor

@BBosman BBosman commented Feb 15, 2019

Pull Request

πŸ“– Description

Upgraded to Typescript 3.3.

🎫 Issues

This is part of #425, which had too many moving parts, so I'm splitting it up.

πŸ‘©β€πŸ’» Reviewer Notes

Notable changes:

πŸ“‘ Test Plan

CircleCI

⏭ Next Steps

Upgrade Chrome on CircleCI, so we can unpin chromedriver.

@codeclimate
Copy link

codeclimate bot commented Feb 15, 2019

Code Climate has analyzed commit ebefe7b and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 90.6% (-1.6% change).

View more on Code Climate.

@codecov
Copy link

codecov bot commented Feb 15, 2019

Codecov Report

Merging #440 into master will decrease coverage by 1.44%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #440      +/-   ##
==========================================
- Coverage   91.67%   90.22%   -1.45%     
==========================================
  Files         139      139              
  Lines       12883    11293    -1590     
  Branches     2402      795    -1607     
==========================================
- Hits        11810    10189    -1621     
- Misses       1073     1104      +31
Impacted Files Coverage Ξ”
packages/runtime/src/binding/call.ts 73.91% <0%> (-18.4%) ⬇️
packages/jit-html-browser/src/index.ts 85.71% <0%> (-14.29%) ⬇️
packages/runtime-html/src/html-renderer.ts 87.23% <0%> (-12.77%) ⬇️
packages/runtime/src/binding/ref.ts 61.53% <0%> (-11.8%) ⬇️
packages/runtime/src/binding/let-binding.ts 72.41% <0%> (-10.92%) ⬇️
packages/runtime-html/src/binding/listener.ts 70.17% <0%> (-10.47%) ⬇️
...ages/runtime/src/observation/primitive-observer.ts 90% <0%> (-10%) ⬇️
...ackages/runtime/src/observation/binding-context.ts 88.63% <0%> (-9.45%) ⬇️
packages/router/src/utils.ts 75% <0%> (-8.34%) ⬇️
...ackages/runtime/src/templating/lifecycle-attach.ts 90.9% <0%> (-7.94%) ⬇️
... and 90 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data
Powered by Codecov. Last update 95d977a...6cf4fc7. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Feb 15, 2019

Codecov Report

Merging #440 into master will decrease coverage by 1.43%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #440      +/-   ##
==========================================
- Coverage   91.76%   90.33%   -1.44%     
==========================================
  Files         139      139              
  Lines       12861    11275    -1586     
  Branches     2396      793    -1603     
==========================================
- Hits        11802    10185    -1617     
- Misses       1059     1090      +31
Impacted Files Coverage Ξ”
packages/runtime/src/binding/call.ts 73.91% <0%> (-18.4%) ⬇️
packages/jit-html-browser/src/index.ts 85.71% <0%> (-14.29%) ⬇️
packages/runtime-html/src/html-renderer.ts 87.23% <0%> (-12.77%) ⬇️
packages/runtime/src/binding/ref.ts 61.53% <0%> (-11.8%) ⬇️
packages/runtime/src/binding/let-binding.ts 72.41% <0%> (-10.92%) ⬇️
packages/runtime-html/src/binding/listener.ts 70.17% <0%> (-10.47%) ⬇️
...ages/runtime/src/observation/primitive-observer.ts 90% <0%> (-10%) ⬇️
...ackages/runtime/src/observation/binding-context.ts 88.63% <0%> (-9.45%) ⬇️
packages/router/src/utils.ts 75% <0%> (-8.34%) ⬇️
...ackages/runtime/src/templating/lifecycle-attach.ts 90.9% <0%> (-7.94%) ⬇️
... and 90 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data
Powered by Codecov. Last update 1f9d50c...ebefe7b. Read the comment docs.

@fkleuver
Copy link
Member

fkleuver commented Feb 15, 2019

@BBosman I was wondering where the decrease in coverage came from, so I ran the tests locally - for some reason there are 30 failing au-compose tests:
image

I am baffled by this.. why would these dependency upgrades have such side effect? :/ any idea what might be causing it?

(btw, we can completely remove ajv dependency.. it was only added to pin it in the first place)

@BBosman
Copy link
Contributor Author

BBosman commented Feb 15, 2019

@fkleuver Weird. I tested locally with npm run test-node and that runs without failure, so I made the (false) assumption that npm run test would as well.

But it appears we must find the reason in whatever is different between those setups and got an upgrade within this PR.

(and I will remove ajv as dependency)

@fkleuver
Copy link
Member

fkleuver commented Feb 15, 2019

@BBosman Well, running locally can be a bit tedious since it takes a few minutes. You can also just rely on CI. The coverage is supposed to stay identical. Fact that it went down is an immediate red flag that something is failing. Why the job doesn't report a failure is another issue and I'm not sure what's going on there. Seems to be running into some kind of timeout

@BBosman
Copy link
Contributor Author

BBosman commented Feb 15, 2019

@fkleuver It keeps getting stranger. A fresh clone of the repo exhibits the same issue.

git clone https://github.com/aurelia/aurelia.git aurelia2
cd aurelia2
npm run bootstrap
npm run build
npm run test

Results in 30 failing tests.

So my PR is surfacing the issue, but is not the root cause?

@BBosman
Copy link
Contributor Author

BBosman commented Feb 15, 2019

Both test-firefox and test-node work, so it has to be something Chrome specific.

@fkleuver
Copy link
Member

Yeah and the build on master still succeeds. I don't know what's going on here, I'll assume it's a weird fluke and if it breaks on master then we'll take it from there. Hunting for ghosts like this is not the most productive way to work on vNext I think you would agree :)
I'll just merge it

@fkleuver fkleuver merged commit 53babc0 into master Feb 16, 2019
@fkleuver fkleuver deleted the ts33 branch February 16, 2019 02:08
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.

None yet

3 participants