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

CI for xdpm with GitHub actions #35

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

pklaschka
Copy link
Contributor

Description

Added a GitHub actions workflow to add some basic CI, meaning Code Review etc. no longer needs everyone to pull the current changes, but just requires looking at the output of the CI.

Currently, I haven't found a way to let the test fail when something that should fail passes (e.g., if xdpm validate passed while outside a plugin dir), but it is a first step into the right direction; I'm more than open for input on how better CI could get achieved for such a CLI.

Related Issue

This somewhat relates to current other PRs where I've been doing QA. Always having to clone the project to check output is kind of annoying and slows down the review process by a lot, but this doesn't necessarily fix any particular issues.

Motivation and Context

How Has This Been Tested?

This is a test by itself. Testing tests would get kind of Meta (who would test this next level of tests, after all), so I've manually tried it 😜 . It is, however, not really functional, meaning it only is required to be a valid GH Actions workflow...

Screenshots (if appropriate):

n/a

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Other (Meta)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation (maybe?).
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes (?).
  • All new and existing tests passed.

@pklaschka pklaschka self-assigned this Nov 26, 2019
@pklaschka pklaschka added the enhancement New feature or request label Nov 26, 2019
@ashryanbeats
Copy link
Member

Thanks Pablo. Trying to have a look. We aren't quite sure how to test/debug an action before merging, so looking into that first.

Actions are new to me so I'm curious: how were you able to run the action while developing it?

@pklaschka
Copy link
Contributor Author

Thanks Pablo. Trying to have a look. We aren't quite sure how to test/debug an action before merging, so looking into that first.

Actions are new to me so I'm curious: how were you able to run the action while developing it?

Of course. You can see the (current) output of it here. It really is committing, looking at the output and iterating. In this case, it wasn't too complicated, as it's basically just running a sequence of commands, but otherwise, I really just commit, wait for the CI logs, look at it, try to correct mistakes and so on over and over again (resulting in very boring commit messages such as Trying to get CI working, Fixing the CI config (probably?) and similar 😉).

Quite honestly, I'm already kind of familiar with Actions (as I had joined the Beta and used them in some server-state-repos), meaning this was done in relatively few commits (and it was basically just adding things I knew how to do) and when I initially tested those first Actions, it really was just as stated above: countless commits leading up to a working CI, so -- there's that 🙂.

I have to say, however: Since GH Actions run quite fast, this is ok as a workflow. I remember building something for GitLab CI and it took me ~15 hours or so to get it working (every change required around 12 minutes of execution in the CI until I got results based on which I could iterate...)

@ashryanbeats
Copy link
Member

OK! I spent a little time getting up to speed on actions and these are really cool. Thanks for start this.

Mind if I contribute some work to the branch on your fork?

@ashryanbeats
Copy link
Member

I haven't found a way to let the test fail when something that should fail passes (e.g., if xdpm validate passed while outside a plugin dir)

Actually, @kerrishotts and I were just looking at this and we think your test and the actions are working as expected. Instead, your test has exposed a bug in xdpm.

From one of your runs:

Run xdpm validate 0s
23
##[error]Process completed with exit code 1.
1
Run xdpm validate
4
INFO: xdpm 3.1.0 - XD Plugin Manager CLI
5
INFO: Use of this tool means you agree to the Adobe Terms of Use at
6
https://www.adobe.com/legal/terms.html and the Developer Additional
7
Terms at https://wwwimages2.adobe.com/content/dam/acom/en/legal/servicetou/Adobe-Developer-Additional-Terms_en_US_20180605_2200.pdf.
8
/home/runner/work/xdpm/xdpm/lib/validate.js:37
9
    manifest.icons.forEach((icon, idx) => {
10
                   ^
11

12
TypeError: Cannot read property 'forEach' of undefined
13
    at validate (/home/runner/work/xdpm/xdpm/lib/validate.js:37:20)
14
    at /home/runner/work/xdpm/xdpm/commands/validate.js:43:24
15
    at Array.map (<anonymous>)
16
    at validatePlugin (/home/runner/work/xdpm/xdpm/commands/validate.js:30:26)
17
    at Object.<anonymous> (/home/runner/work/xdpm/xdpm/index.js:72:39)
18
    at Module._compile (internal/modules/cjs/loader.js:959:30)
19
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:995:10)
20
    at Module.load (internal/modules/cjs/loader.js:815:32)
21
    at Function.Module._load (internal/modules/cjs/loader.js:727:14)
22
    at Function.Module.runMain (internal/modules/cjs/loader.js:1047:10)
23
##[error]Process completed with exit code 1.

In manifest.icons.forEach, icons is undefined so the code is failing.

I'm not yet sure if fixing this in the code will mean that since the script fails in the right way the test will pass, but we'll need to fix the bug either way.

@pklaschka
Copy link
Contributor Author

@ashryanbeats Yes, that's a bug with xdpm, but the problem is that these steps are currently configured as continue-on-error=true, meaning CI reports success even though these tests (expectedly) fail. They should, however, fail, as I've added tests for invalid things to test this aspect of xdpm as well. If this, however, were to pass (which, of course, it shouldn't), the workflow would (wrongly) succeed (while it should actually fail as xdpm had an exit code of 0 where it shouldn't have succeeded)...

Basically, my CI workflow currently has a problem with false negatives in xdpm's manifest validation, if there were any...

@ashryanbeats
Copy link
Member

ashryanbeats commented Dec 12, 2019

Thanks @pklaschka. I'm starting to wonder if the point of GitHub Actions is that we should be writing things that are supposed to pass, rather than checking for things that are supposed to fail (although I too want to do both).

I did a little hacking on the actions you set up:
https://github.com/ashryanbeats/xdpm/blob/github-actions/.github/workflows/cli-test.yml

In that fork, the changes worth noting:

  • [action] Now running jobs on Mac and Win
  • [action] Ensuring xdpm install action doesn't fail due to non-existent target dirs by using mkdir/md
  • [lib/validate.js] Ensuring xdpm validate action doesn't fail by checking if .icons property existing before invoking .forEach

Here's the result:
https://github.com/ashryanbeats/xdpm/runs/346161827

You'll note that there are no more failure annotations. This may not be the final result we're looking for, but I'm hoping this is helpful.

@ashryanbeats
Copy link
Member

ashryanbeats commented Dec 12, 2019

Another thought to keep in mind is that the bootstrap command, when available in 4.0, should be quite useful in terms of running the other xdpm commands for success scenarios.

@ashryanbeats
Copy link
Member

Note: edited 2 comments up to link to the correct branch's yml file.

@pklaschka
Copy link
Contributor Author

@ashryanbeats Oops, sorry. You'll have to open another PR into pklaschka:github-actions to get reflected here. My master branch has some experimental Linux Subsystem for Windows stuff in it, too, cf. #21. We've accidentally merged this into my master-branch, which is therefore not usable for this PR...

@ashryanbeats ashryanbeats mentioned this pull request Dec 13, 2019
3 tasks
@pklaschka
Copy link
Contributor Author

pklaschka commented Jan 3, 2020

Curent CI results: https://github.com/pklaschka/xdpm/runs/372148630

@alexandrtovmach
Copy link
Contributor

LGTM, but I'd like to write a test coverage instead of continue-on-error=true trick. Probably it's can be merged, to at least validate PRs in a way "it builds successfully or not?"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants