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

docs: fix BannerComponent unit tests #42336

Closed
wants to merge 2 commits into from
Closed

docs: fix BannerComponent unit tests #42336

wants to merge 2 commits into from

Conversation

swseverance
Copy link
Contributor

remove async and await from BannerComponent test because the
component uses an inline template and styles

create new region in banner-external.component.spec.ts demonstrating
test setup that may fail due to a missing call to .compileComponents()
for a component with an external template and stylesheet

resolves #42325

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #42325

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@google-cla google-cla bot added the cla: yes label May 25, 2021
@pullapprove pullapprove bot requested a review from AndrewKushnir May 25, 2021 23:20
@swseverance
Copy link
Contributor Author

@petebacondarwin

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

@swseverance thanks for the fixing this, the change looks good to me (just one minor comment) 👍

I'm also adding @petebacondarwin as a reviewer since he may have more context on this.

// #docregion setup-may-fail
beforeEach(async () => {
await TestBed.configureTestingModule({
declarations: [BannerComponent],
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the formatting in these files leave a space in front and right after a symbol, so I believe this should look like:

Suggested change
declarations: [BannerComponent],
declarations: [ BannerComponent ],

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AndrewKushnir thanks for taking a look. Within banner-external.component.spec.ts there are a total of three declarations: [BannerComponent] statements (only one of which I introduced in this PR). Would you like me to change all three to declarations: [ BannerComponent ]?

Copy link
Contributor

Choose a reason for hiding this comment

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

Within banner-external.component.spec.ts there are a total of three declarations: [BannerComponent] statements (only one of which I introduced in this PR). Would you like me to change all three to declarations: [ BannerComponent ]?

Thanks for the comment. Let's keep it as is for now and see what @petebacondarwin suggests.

Copy link
Member

Choose a reason for hiding this comment

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

The formatting of the examples is a bit ad hoc since we have not, so far, chosen or enforced a specific formatter tool.
Until we do that, we should try to keep the formatting consistent within guides. The arrays in the the banner.component.spec.ts file contain spaces around the element, therefore I think that @AndrewKushnir is correct here. Please can you update all the declarations in this file to match.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@petebacondarwin I made the suggested changes. thanks!

@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer comp: docs target: patch This PR is targeted for the next patch release labels May 25, 2021
@ngbot ngbot bot modified the milestone: Backlog May 25, 2021
@mary-poppins
Copy link

You can preview 8ab5366 at https://pr42336-8ab5366.ngbuilds.io/.

Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Thanks for this update @swseverance - I am approving subject to making the change to the declarations array formatting.

Also while you are here, I note that this example code:

// #docregion async-before-each
beforeEach(async () => {
  TestBed
    .configureTestingModule({
      declarations: [BannerComponent],
    })
    .compileComponents();  // compile template and css
});
// #enddocregion async-before-each

should probably have an await in front of the call to TestBed.configureTestingModule(...).compileComponents(), right? Perhaps you could fix that in an extra commit in this PR?

@petebacondarwin petebacondarwin added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels May 26, 2021
remove `async` and `await` from `BannerComponent` test because the
component uses an inline template and styles

create doc region in `banner-external.component.spec.ts` demonstrating
test setup that may fail due to a missing call to `.compileComponents()`
for a component with an external template and stylesheet
@mary-poppins
Copy link

You can preview 978f3b1 at https://pr42336-978f3b1.ngbuilds.io/.

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

@swseverance thanks for applying the changes to make the code more consistent 👍

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels May 26, 2021
@zarend zarend closed this in caf15da May 26, 2021
zarend pushed a commit that referenced this pull request May 26, 2021
zarend pushed a commit that referenced this pull request May 26, 2021
remove `async` and `await` from `BannerComponent` test because the
component uses an inline template and styles

create doc region in `banner-external.component.spec.ts` demonstrating
test setup that may fail due to a missing call to `.compileComponents()`
for a component with an external template and stylesheet

PR Close #42336
zarend pushed a commit that referenced this pull request May 26, 2021
umairhm pushed a commit to umairhm/angular that referenced this pull request May 28, 2021
remove `async` and `await` from `BannerComponent` test because the
component uses an inline template and styles

create doc region in `banner-external.component.spec.ts` demonstrating
test setup that may fail due to a missing call to `.compileComponents()`
for a component with an external template and stylesheet

PR Close angular#42336
umairhm pushed a commit to umairhm/angular that referenced this pull request May 28, 2021
iRealNirmal pushed a commit to iRealNirmal/angular that referenced this pull request Jun 4, 2021
remove `async` and `await` from `BannerComponent` test because the
component uses an inline template and styles

create doc region in `banner-external.component.spec.ts` demonstrating
test setup that may fail due to a missing call to `.compileComponents()`
for a component with an external template and stylesheet

PR Close angular#42336
iRealNirmal pushed a commit to iRealNirmal/angular that referenced this pull request Jun 4, 2021
@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 Jun 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker aio: preview cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docs: BannerComponent test setup issue
4 participants