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

No amp4test log on travis. #19611

Merged
merged 9 commits into from Dec 21, 2018
Merged

No amp4test log on travis. #19611

merged 9 commits into from Dec 21, 2018

Conversation

lannka
Copy link
Contributor

@lannka lannka commented Dec 4, 2018

Still want logs on local testing.

Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Unfortunately, this was polluting the logs during local debugging test runs as well. Perhaps you can guard it with a flag that you pass in only if you actually care about the logs?

@lannka
Copy link
Contributor Author

lannka commented Dec 4, 2018

I think it's fine? I assume running test locally is most in dev / debug mode?
(I haven't been running all tests locally for years)

@rsimha
Copy link
Contributor

rsimha commented Dec 4, 2018

Running all tests locally is a routine workflow for the infra team, so these logs do come in the way due to how often they're printed.

A couple suggestions:

  1. Change the AMP_TEST var in runtime-test.js to something like ENABLE_A4A_LOGGING, and enable it only when a --a4a-logging flag is passed in.
  2. Write the logs to a local file (added to .gitignore) instead of to the console, and verify that the logs are present.

@lannka
Copy link
Contributor Author

lannka commented Dec 4, 2018

@rsimha instead of introducing another flag, what's your attitude to reusing the existing --files= flag? Assuming when tests run alone we are OK with verbal logs?

@rsimha
Copy link
Contributor

rsimha commented Dec 4, 2018

Good idea, using --files sgtm.

@lannka
Copy link
Contributor Author

lannka commented Dec 21, 2018

@rsimha changed to read --files. PTAL

Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

LGTM with one minor cleanup task.

* @param {*} messages
*/
function log(...messages) {
if (!process.env.AMP_TEST) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If this environment variable isn't used anywhere else, it can be removed from runtime_test.js.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. thx

* @param {*} messages
*/
function log(...messages) {
if (!process.env.AMP_TEST) {
console.log(messages);
if (argv.files) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I just realized we also have a verbose flag. You could use that instead of (or in addition to) files if you'd like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SG. switched to --verbose

@lannka lannka merged commit c83e901 into ampproject:master Dec 21, 2018
noranazmy pushed a commit to noranazmy/amphtml that referenced this pull request Mar 22, 2019
* No amp4test log on travis.

* read --files to enable logging

* Update js doc

* clean up

* print error in correct format

* also do --verbose

* use --verbose

* Update js doc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants