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.

Sign up for GitHub

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

Jump to bottom

test(ivy): add JIRA references for root-casuse TestBed failures #27196

Closed
wants to merge 1 commit into from
Closed

test(ivy): add JIRA references for root-casuse TestBed failures #27196

wants to merge 1 commit into from

Conversation

pkozlowski-opensource
Copy link
Member

This PR enables more of the @angular/core tests running with ivy on TestBed. It basically do 2 things:

  • moves fixmeIvy from describe to it to enable more tests running on CI (for cases where there were passing tests but there were not running since the entire describe was disabled);
  • adds root cause analysis where one can be derived from the error message (there are still many cases where investigation is needed to actually understand the root cause)

@mary-poppins
Copy link

You can preview 074333e at https://pr27196-074333e.ngbuilds.io/.

@pkozlowski-opensource pkozlowski-opensource added action: review The PR is still awaiting reviews from at least one requested reviewer comp: ivy labels Nov 20, 2018
@ngbot ngbot bot added this to the needsTriage milestone Nov 20, 2018
@pkozlowski-opensource pkozlowski-opensource added the target: major This PR is targeted for the next major release label Nov 20, 2018
@mary-poppins
Copy link

You can preview af7a4ea at https://pr27196-af7a4ea.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 9ae3cc8 at https://pr27196-9ae3cc8.ngbuilds.io/.

@mary-poppins
Copy link

You can preview d4fcb3c at https://pr27196-d4fcb3c.ngbuilds.io/.

kara
kara reviewed Nov 20, 2018
View reviewed changes
Copy link
Contributor

@kara kara 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 except for these nits.

p.s. also noticed a typo in the commit message. fix?

packages/core/test/linker/integration_spec.ts Outdated
@@ -1412,7 +1434,7 @@ function declareTests(config?: {useJit: boolean}) {
expect(getDOM().querySelectorAll(fixture.nativeElement, 'script').length).toEqual(0);
});

it('should throw when using directives without selector', () => {
fixmeIvy('pull/27169') && it('should throw when using directives without selector', () => {
Copy link
Contributor

@kara kara Nov 20, 2018 •
edited

Choose a reason for hiding this comment

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

I think it might be good to keep this fixme consistent with the rest and use the JIRA issue number and title here. (Same with PR mentions below)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

packages/core/test/linker/query_integration_spec.ts Outdated
beforeEach(
() =>
TestBed
.configureTestingModule({
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to fix the formatting here? It's a bit hard to read

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, all credits go to clang :-)

Exceptionally, for this particular one, I've just added ticket reference (FW-670) and the full ticket title in a comment above.

packages/core/test/linker/regression_integration_spec.ts Outdated
const token = 'a.b';
const tokenValue = 1;
const injector = createInjector([{provide: token, useValue: tokenValue}]);
fixmeIvy('FW-646: Cannot create property __NG_ELEMENT_ID__ on a primitive type') &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change so the reason matches the JIRA ticket title?

FW-646: Directive providers don't support primitive types as DI tokens

@kara kara added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews PR action: time-zone labels Nov 20, 2018
AndrewKushnir
AndrewKushnir reviewed Nov 20, 2018
View reviewed changes
packages/core/test/linker/ng_container_integration_spec.ts Outdated
@@ -15,10 +15,11 @@ import {fixmeIvy} from '@angular/private/testing';

{
if (ivyEnabled) {
fixmeIvy('unknown') && describe('ivy', () => { declareTests(); });
fixmeIvy('FW-663: ReferenceError: goog is not defined') &&
Copy link
Contributor

Choose a reason for hiding this comment

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

@pkozlowski-opensource I'd propose to move this fixmeIvy next to the should support the "i18n" attribute test (which is causing this error), the rest of the tests should not have this issue and can be investigated further for other failures.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. You are right, goog is not defined issue is blocking just one test here.

@mary-poppins
Copy link

You can preview 71a4507 at https://pr27196-71a4507.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 15fe3f1 at https://pr27196-15fe3f1.ngbuilds.io/.

@pkozlowski-opensource pkozlowski-opensource removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Nov 21, 2018
@pkozlowski-opensource pkozlowski-opensource removed their assignment Nov 21, 2018
@mhevery mhevery added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Nov 21, 2018
@mhevery mhevery closed this in d666370 Nov 21, 2018
@pkozlowski-opensource pkozlowski-opensource mentioned this pull request Nov 22, 2018
Closed
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
@pkozlowski-opensource @FrederikSchlemmer
test(core): add JIRA references for root-casuse ivy TestBed failures (a…
fe80691 
…ngular#27196)

PR Close angular#27196
ngfelixl pushed a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019
@pkozlowski-opensource @ngfelixl
test(core): add JIRA references for root-casuse ivy TestBed failures (a…
b219f04 
…ngular#27196)

PR Close angular#27196
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kara kara kara left review comments

@AndrewKushnir AndrewKushnir AndrewKushnir left review comments

@mhevery mhevery mhevery approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@pkozlowski-opensource @mary-poppins @mhevery @kara @AndrewKushnir @googlebot