Skip to content

Conversation

@devversion
Copy link
Member

Instead of remembering what individual browser targets are called,
and what the convention for Bazel labels is, we should just provide
a script that simplifies the Bazel testing workflow. e.g.

yarn test button [--local] [--firefox] [--no-watch]

yarn test src/material/button src/cdk/bidi
yarn test button slider coercion

Eventually we should document this script better in one of the
contributing markdown files, but these docs need a general cleanup
since they still mention Gulp.

@devversion devversion added pr: merge safe target: patch This PR is targeted for the next patch release labels Dec 20, 2019
@devversion devversion requested a review from jelbourn as a code owner December 20, 2019 14:33
@devversion devversion requested a review from a team December 20, 2019 14:33
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 20, 2019
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM

Instead of remembering what individual browser targets are called,
and what the convention for Bazel labels is, we should just provide
a script that simplifies the Bazel testing workflow. e.g.

```bash
yarn test button [--local] [--firefox] [--no-watch]

yarn test src/material/button src/cdk/bidi
yarn test button slider coercion
```

Eventually we should document this script better in one of the
contributing markdown files, but these docs need a general cleanup
since they still mention Gulp.
@devversion devversion force-pushed the build/script-to-run-tests branch from 69ad269 to f30edba Compare December 31, 2019 12:51
*
* Supported command line flags:
*
* --local | If specified, no browser will be launched.
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this the default? Then invert the flag to something like --no-headless

Copy link
Member Author

@devversion devversion Jan 12, 2020

Choose a reason for hiding this comment

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

Do you mean that we do not run any browser by default? (asking because the --local flag does this and you commented on that flag).

If you meant that we run Chrome in non-headless mode by default: I don't think we have a way to launch Chrome through Bazel without headless (it seems to depend on the $DISPLAY env variable).

Copy link
Member

Choose a reason for hiding this comment

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

I want the command to not pop open a browser window but to also run the tests without having to manually open the page (but you can still open the debug page if you need to)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this is possible. The @bazel/karma rules decide based on the platform environment whether the browser runs in headless or non-headless mode. For me on Windows, it always runs headless.

Copy link
Contributor

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

This is great! Would love to see at least a minor mention in the Contributing docs.

@devversion
Copy link
Member Author

@Splaktar Yeah. I have an item on my list to update it to match our latest changes. IIRC it still references Gulp etc. If you want to have a look at the doc, that would be cool too 😄

@devversion
Copy link
Member Author

@jelbourn The PR that supposedly fixed the _local targets landed in master. I think this is ready for another round of review. Note that the _local target doesn't work out nicely on Windows though (due to a Bazel 1.0.0 change in behavior). Greg reported an issue on the Bazel repo for this. There is also an easy workaround on windows.

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

We can see about the upstream behavior as a separate concern

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jan 26, 2020
@jelbourn jelbourn merged commit aa2a5b3 into angular:master Jan 26, 2020
yifange pushed a commit to yifange/components that referenced this pull request Jan 30, 2020
…18016)

Instead of remembering what individual browser targets are called,
and what the convention for Bazel labels is, we should just provide
a script that simplifies the Bazel testing workflow. e.g.

```bash
yarn test button [--local] [--firefox] [--no-watch]

yarn test src/material/button src/cdk/bidi
yarn test button slider coercion
```

Eventually we should document this script better in one of the
contributing markdown files, but these docs need a general cleanup
since they still mention Gulp.
@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 Feb 26, 2020
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 cla: yes PR author has agreed to Google's Contributor License Agreement target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants