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

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

Merged
merged 1 commit into from
Sep 24, 2018
Merged

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

merged 1 commit into from
Sep 24, 2018

Conversation

wigglemuff
Copy link
Contributor

@wigglemuff wigglemuff 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

@wigglemuff wigglemuff added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it Docker labels Sep 21, 2018
@wigglemuff wigglemuff requested a review from a team as a code owner September 21, 2018 07:36
@wigglemuff wigglemuff added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Sep 21, 2018
@jetpackbot
Copy link

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
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
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.

@wigglemuff
Copy link
Contributor 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
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
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 To request a review from fellow Jetpack developers. Label will be renamed soon. 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 [Type] 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