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

Environment: check GD and Imagick availability & supported formats. #134

Merged

Conversation

adamsilverstein
Copy link
Member

Addresses #131.

prepare.php Outdated Show resolved Hide resolved
@getsource
Copy link
Member

Thanks so much! I haven't had the chance to test yet, but this looks very close.

Due to #111 , unfortunately, this needs to be updated in two places at the moment -- both prepare.php and functions.php.

I need to resurrect that issue / patch / diff at some point (or help is welcome) to refactor and turn it into an optional SSH call for gathering environment data, but I think it's fine to go ahead and add this in the meantime.

@adamsilverstein
Copy link
Member Author

Due to #111 , unfortunately, this needs to be updated in two places at the moment -- both prepare.php and functions.php.

I missed that, will add there as well. Thanks for the tip.

@adamsilverstein
Copy link
Member Author

Addressed feedback:

  • Added to functions.php.
  • Lower case naming.

Copy link
Member

@pfefferle pfefferle left a comment

Choose a reason for hiding this comment

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

LGTM.

@adamsilverstein
Copy link
Member Author

@getsource friendly ping, this should be ready to go!

@getsource
Copy link
Member

Thanks! Trying to test, but ran into some environment difficulties with app passwords and the reporter plugin.
Will ping back when I've been able to test.

Copy link
Member

@getsource getsource left a comment

Choose a reason for hiding this comment

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

I tested, and the information gets uploaded to the reporter as expected.

A couple of notes:

  • We'll need to add something to the reporter to display this information (Edit: This does not have to happen before this gets merged, but we should probably open an Issue either now, or when it does).
  • When successful, an array gets inserted for both items. Would an empty array (or not including it at all / "empty") make it easier to parse this in the reporter when "unavailable"?

@getsource
Copy link
Member

@adamsilverstein
Copy link
Member Author

I created an issue to add the data if available to the reports: WordPress/phpunit-test-reporter#97

When successful, an array gets inserted for both items. Would an empty array (or not including it at all / "empty") make it easier to parse this in the reporter when "unavailable"?

Good point, I hadn't considered the reporting end of things. Makes more sense for the default value to be an empty array vs. the string 'unavailable'. I will update the PR.

@adamsilverstein
Copy link
Member Author

@getsource - I changed the defaults to empty array as per your suggestion. This should be good to merge.

@adamsilverstein
Copy link
Member Author

@pfefferle @getsource What are the next steps to get this merged (plus WordPress/phpunit-test-reporter#98).

Also: once this is merged, how is deployed in the wild, when could we hope to start seeing data reported?

@getsource
Copy link
Member

My suggested steps would be:

  • Review of this PR's code
  • Review of the other PR's code
  • Testing both of the two together (WP.org meta environment for the latter).
  • Merge of both PRs
  • Create New Plugin Version for the reporter
  • Create meta patch for the reporter PR (using the reporter build tools)
  • Make meta ticket, with that patch attached
  • Commit from the meta team to make the new version of the reporter live

I don't think this PR necessarily needs to wait for the latter, but having them work together as a proof of concept definitely would help verify/validate.

I don't recall whether sending the data to the reporter when it isn't expecting it (if this PR is merged first) would cause validation issues with the API or not, but looking+testing to make sure it works as expected would be good before this is merged.

As far as when data is received: After the above is done, because updating the runner code is dependent on the client, I suspect some hosts will start to report right away (if they clone this repo as part of the testing process), and for others it will take an undefined amount of time until they decide to manually update.

I don't have the time at the moment to work on this, unfortunately, so help is appreciated.

@desrosj
Copy link
Contributor

desrosj commented May 30, 2023

Just cross referencing the recent Core changeset that added this data to update pings to .org: https://core.trac.wordpress.org/changeset/53753.

@javiercasares javiercasares merged commit fb00cbe into WordPress:master Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants