Download PR-specified plugins in Travis-CI build

[SOF3]

Description

This pull request changes the Travis build script. The build will download a list of plugins specified in the pull request body, and run the test server with them. The plugin list section starts with this line:

# Plugins to test with Travis-CI                                                                                                                 <!-- IGNORE THIS COMMENT - this comment is for INTERNAL use -->

Followed by a plugin list, where plugins can be specified in these formats:

- http://url.here
- [file-name.php](https://url.here)

If the former format is used, a file Genisys-CI-Untitled-%d.%s will be created, where %d is the line number - 1 of the line that specified the plugin in the pull request body, and %s is php or phar, detected in this algorithm:

if __HALT_COMPILER exists in the file
then
    attempt to create Phar object from this file
    if UnexpectedValueException is thrown
    then
        implies that it is not a valid phar
        set file extension to .php
    else
        set file extension to .phar
    end if
else
    set file extension to .php
end if

The plugin list section is terminated by:

• a line exactly <!-- END --> when trimmed
• a line without visible characters (strlen(trim($line)) === 0)
• a line that does not match any of the two formats above
• end of file

And skips but is not terminated by other lines starting with <!--.

Examples are present as followings:

Plugins to test with Travis-CI

• https://gist.githubusercontent.com/SOF3/04cf1f3ced0d062a3bf7578ab086e692/raw/738f5aaaa6b58c6ba6a01b025a77f8affbcdb3a1/plugin.php

Reason to modify

Travis-CI is usually useless for pull requests related to plugin API. In order to maximize the

• https://gist.githubusercontent.com/SOF3/7ab7222ec3c76bd3c5dad73fc26b0cb4/raw/21a6f327b5cf550e1dbe77bb08d81ce1f3fb66f4/plugin.php
  usage of Travis-CI, this pull request allows future pull requests to validate that they work through attaching links to plugins that can test against their features. These can be instant script plugins on GitHub Gist/other pastebin sites, or preexisting plugins that are known to use a certain API feature.

Plugins can also be created to stress test the server from different approaches.

Tests & Reviews

I have tested ciDlPlugins.php and it works.

I have not tested how the modified .travis.yml works on Travis-CI yet, especially when it comes to GitHub API rate limit. However, through careful manual inspection, it seems to be correct.

Possible enhancements/fixes

• Fix issue about GitHub API rate limit on Travis-CI
  • Setup a proxy page on a server officially specialized for Genisys, which will help access GitHub API with a private GitHub access token
• Allow more syntax flexibility when listing the plugins
• Test server twice, once with plugins, once without plugins. Probably even checkout BASE and run test with plugins there as well.
• Extend this feature to normal commits (but probably would never get useful) CI on the master branch is more useful to test the stability of the software rather than the stability of an individual commit. wontfix
• Load from attached files in pull request main comment GitHub doesn't allow uploading .php files

[ishitatsuyuki]

I don't really like this idea.
Mostly it can cause the situation: "uh, there's a plugin in CI, let's not evolve the API."

[SOF3]

@ishitatsuyuki can you provide examples to elaborate your point?

[ishitatsuyuki]

Sure, the win10 inventory API update is one. Also, many plugin for PM are poorly written, since it's PHP and its code quality can barely compare to a server software.

[SOF3]

@ishitatsuyuki although it is possible to load existing phar plugins, it is recommended that pull request authors write specific script plugins, paste them on GitHub Gist and include them in their pull requests. This allows more straightforward testing through plugins.

[dktapps]

I think the intention is more along the lines of adding test plugins to test certain capabilities after modifications, written purely for this purpose. Currently the CI just runs, builds, shuts down. That does not pickup anything that breaks elsewhere unless it's something very basic like a syntax error. Used correctly, this can be used to demonstrate the results of changes made in a PR.

[ishitatsuyuki]

Not really. The real thing is if the user can receive the correct packet, not the plugins.

[SOF3]

Look at #1845 for example. If I make a plugin as simple as one that uses saveResource() properly, the Travis build means much more.

[SOF3]

I have written a simple script that can act as a proxy page according to an environment variable containing the access token:
https://gist.github.com/SOF3/06c085d8bbf69b44b16acceca8711509

[ishitatsuyuki]

I don't like PHP, personally I think it's better written with shell or Python.

[SOF3]

@ishitatsuyuki I doubt if it is easily possible to parse the message using shell. Would probably be much more complicated. Since the whole project is in PHP and the test also uses PHP, it is natural to use PHP for parsing the PR message.

[dschwartz783]

More complicated? Most things are simpler to do from the shell, if you know the commands to use. All that overly verbose PHP curl stuff would be boiled down into a few flags from the CLI, and you can use jq to parse the resulting json data. Or a tiny snippet of (preferably) Python, or PHP if you want, because most probably don't have that installed...

[SOF3]

@dschwartz783 that's exactly what I'm talking about. jq probably needs to be installed through aptitude, right? I want to minimize the amount of components to install per build. Trying to use all resources in hand, rather than constantly adding new ones.

[ghost]

PHP sucks, but it doesn't matter. You've already got it installed, just use this script since it's already written. Don't even bother complaining, then you just get nothing done (end hypocrisy)

[SOF3]

@mhsjlw the main complaint is that this kind of stuff should be done through unit tests instead. My argument is that it is probably not good to create a unit test for everything as minor as a one-line bugfix pull request. The unit tests will stack and become unsustainable if they are used for small things like this. Unit tests are just for something different ‐ something that is probably unstable, not something that is previously wrong and now correct, where such test would only get useful for a while and is not worth being kept forever.

[ishitatsuyuki]

Please give an argument. I think it's pointless and just making the CI longer.

[dktapps]

To be honest, who is really going to put the effort into writing test plugins for a one-liner? As long as a suitable, logical explanation of changes is provided, I think that would be preferable to a test plugin also written by the person making the PR. I also think this is potentially open to abuse.

[SOF3]

@dktapps what kind of abuse? Generate a phar from the external plugin and call a failed artifact successful?

[dktapps]

@SOF3 producing false test results by making it appear to be doing something it's not. Also, there's bias to consider too. How can we trust people to make a PR with test plugins that test realistic scenarios, and not just one that shows things in a favourable light?

[ishitatsuyuki]

Uh, two opposing opinions. Thank you very much for coding, however we're going to close this.

[SOF3]

@dktapps because you can look at the test plugin? The test plugin is available to everyone and should be at a static (permanent and unchanged) link, such as a script plugin on GitHub Gist raw. It is not difficult to look at what is being tested at all.

Also, as the pull request body above mentioned, it should be improved such that it runs the CI both with and without the plugin.

@ishitatsuyuki I simply don't see what's the disadvantage of adding this. I believe the PR handlers of Genisys are not stupid enough to be fooled by a script plugin anyway, especially when such test script plugins should be as simple as possible to reflect what is being tested.

[dktapps]

@SOF3 You think the reviewers aren't stupid enough? I'm terribly sorry to disappoint you... it has happened many times in the past. That's why we now have such strict code reviewing standards. Some of our reviewers have been... less than stringent in the past, let's say. One of the reasons I'm considering moving my new fork outside of iTX.