Skip to content

More constants added to error log for the Jetpack test suite#10193

Merged
jeherve merged 1 commit intoAutomattic:masterfrom
rakmob42:fix/docker-error-log-constants
Sep 24, 2018
Merged

More constants added to error log for the Jetpack test suite#10193
jeherve merged 1 commit intoAutomattic:masterfrom
rakmob42:fix/docker-error-log-constants

Conversation

@rakmob42
Copy link
Copy Markdown

@rakmob42 rakmob42 commented Sep 21, 2018

Fixes #9773

Changes proposed in this Pull Request:

Testing instructions:

  • You need to output to log from test files e.g. with error_log('boom')
  • Instructions for running tests in Docker setup
  • Tail logs from Docker setup with yarn docker:tail while running those tests — you should see "boom" with this change. Without the change you don't see it.

Proposed changelog entry for your changes:

  • N/A

@rakmob42 rakmob42 added Enhancement Changes to an existing feature — removing, adding, or changing parts of it Docker labels Sep 21, 2018
@rakmob42 rakmob42 requested a review from a team as a code owner September 21, 2018 07:36
@rakmob42 rakmob42 added the [Status] Needs Review This PR is ready for review. label Sep 21, 2018
@jetpackbot
Copy link
Copy Markdown
Collaborator

That's a great PR description, thank you so much for your effort!

Generated by 🚫 dangerJS

@simison simison requested a review from a team September 21, 2018 07:56
@mdawaffe
Copy link
Copy Markdown
Member

Do we want to be running tests with CONCATENATE_SCRIPTS off and SCRIPT_DEBUG on? That's a different environment than most people use in production? I don't know if it matters :)

@kraftbj
Copy link
Copy Markdown
Contributor

kraftbj commented Sep 21, 2018

I suppose it depends on if we're testing the concatenation or not. If there are problems, the errors would be more direct to what on our end is broken. I see your point though. Not sure either.

@rakmob42
Copy link
Copy Markdown
Author

I read up about this here: https://stackoverflow.com/questions/32588692/how-does-concatenate-scripts-work-on-wp-config/32589922

And I found this info:

by defining the constant as false you are practically forcing WordPress to load each script on the administration page individually rater than collectively. So in that case - If one script fails to load and work correctly, the others can still continue to operate correctly.

This is a recommended setting for debugging and local development

I guess that's why @jeherve's asked to add it (for debugging).

That's a different environment than most people use in production?

Would that be an issue? I don't think so. @jeherve can add more to this I guess? :)

@jeherve
Copy link
Copy Markdown
Member

jeherve commented Sep 24, 2018

That's a different environment than most people use in production? I don't know if it matters :)

I don't know; I personally like to switch back and forth when testing; CONCATENATE_SCRIPTS and SCRIPT_DEBUG can be helpful to get to the source of an issue.

Copy link
Copy Markdown
Contributor

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

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

Let's roll with this as I think it would help in reporting more actionable errors. If we run into any situations where this causes us more harm, we can revert. Since it is just in the testing environment, we can be a little more liberal to let it merge vs code that runs in production anywhere.

@kraftbj kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Sep 24, 2018
@kraftbj kraftbj added this to the 6.6 milestone Sep 24, 2018
@jeherve jeherve merged commit 014bbcf into Automattic:master Sep 24, 2018
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Sep 24, 2018
jeherve added a commit that referenced this pull request Sep 25, 2018
jeherve added a commit that referenced this pull request Sep 25, 2018
* Readme: add boilerplate for next release, 6.6

* Add 6.5 to the changelog.txt file

* Set boilerplate testing list for 6.6

* Readme: update stable tag to 6.5

* Add bullets to 6.5 changelog items

* Readme: add link to previous changelogs

This will help folks who want to know more about past releases,
while keeping the readme.txt short so as to not overwhelm translators and site owners only looking for information about the last release.

* Changelog: add information at the top of the changelog file.

* Changelog: add #10054

* Changelog: add #10078

* Changelog: add #10079

* Changelog: add #10064

* Changelog: add #10094

* Changelog: add #10096

* Testing list: add more information based on #10087

* Changelog: add #9847

* Changelog: add #10084

* Changelog: add #9918

* Changelog: add #7614

* Changelog: add #10116

* Changelog: add #10108

* Changelog: add #10041

* Changelog: add #10121

* Changelog: add #10134

* Changelog: add #10130

* Changelog: add #10109

* changelog: add #10137

* changelog: add #9952

* changelog: add #10120

* changelog: add #10162

* Changelog: add #10163

* Changelog: add #10092

* changelog: add #10156

* Changelog: add #10154

* changelog: add #10122

* Changelog: add #10101

* changelog: add #10105

* changelog: add #10190

* Changelog: add #10196

* changelog: add #10152

* Changelog: add #10153

* Testing list: add more details to Site Verification testing steps.

@see #10143 (comment)

* changelog: add #10194

* Changelog: add #10193
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Docker Enhancement Changes to an existing feature — removing, adding, or changing parts of it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants