ckeditor / ckeditor4-workflows-common

Shared CKEditor 4 GitHub workflows.
1 stars 2 forks source link

Add semi-automatic tests script #15

Closed sculpt0r closed 3 years ago

sculpt0r commented 3 years ago

I create a script that uses GH CLI & GH API

Requirements: pre-prepared repository with at least one commit.

Workflow:

  1. Switch to your working branch at ckeditor4-workflows-common
  2. Run ./test.sh
    1. Reset a master branch to a given commit. It could be any commit that leaves the master branch in the state needed for testing.
    2. Delete all running (not queued!) actions
    3. Commit all files inside workflows directory and optionally workflows-settings.json to target testing repository.
    4. Gather and dispatch every action. Please notice, that actions have to be dispatchable

This script could be used in the current master branch state. If we provide more .yml's that require e.g. push an event to be triggered - we will have to upgrade this script.

The script needs to be modified inside, at least with the testing repository and default init commit hash. So if we need to test workflow in a certain repo state - we can provide such a commit and the master branch will be always reset to this point.

The last point is to unfortunately manually check logs for each workflow. However, it's still faster to set up the initial environment & upload workflows files.

Closes #14

f1ames commented 3 years ago

Workflow:

Switch to your working branch at ckeditor4-workflows-common,

Run ./test.sh

  1. Reset a master branch to a given commit. It could be any commit that leaves the master branch in the state needed for testing.
  2. Delete all running (not queued!) actions
  3. Commit all files inside workflows directory and optionally workflows-settings.json to target testing repository.
  4. Gather and dispatch every action. Please notice, that actions have to be dispatchable.

That's a good start and you have already proven that it makes testing easier in #12 :+1:, but I think it will be good to polish testing procedure slightly. There are few problematic assumptions here:

We should go with the simplest procedure possible but at the same time make sure it's stable, workflows will not be conflicting one another and new tests could be added easily. So from my perspective:

  1. Instead of resetting test branch, maybe we can reset its contents by simply pushing new commit there? That way the commit can have proper description (like "Test workflow something, something") and then it will be clearly visible at what branch state each workflow was launched.
  2. Workflows should be launched in a sequence so we are sure there are now two workflows executing at the same time and interfering each other.
  3. Each workflow has specific outcome. But the first thing we can check automatically is if workflow was successful (so exit code 0). I hope GH API allows that (looks like it could be done). If as a result workflow creates a branch or PR we can check if it was created. Or if new commit was pushed to specific branch. I think as a first step we could focus on checking if workflow was successful only and then see how it works and what can be improved.

As for more details on the above:

Resetting branch contents

It will be good to do this without force pushing so we have entire history preserved for easier workflows "debugging". I think it could be done like:

git clone test-repo
cd test-repo
rm -rf *
// move files required for specific test
git add .
git commit -m "Test..."
git push origin branch-name

This way for each test we will have files required but at the same time the history is preserved and can be checked later. TBH this looks like a beforeEach step so probably should be run before each test.

Launching workflows in a sequence and checking its result

I would imagine each workflow run as a separate test in a test suite - so beforeEach is run then a single test (dispatching workflow and waiting for it to end) and then go to next test/workflow. This prevents workflows from interfering each other and makes it possible to run single workflow more than once with different configuration.

And naturally this requires checking if and how each workflow ended. As mentioned above - I think as a first step we could focus on checking if workflow was successful only and then see how it works and what can be improved.

Further issues

For now I see at least two issue with the current and proposed approach:


That's my suggestions only, so feel free to propose different approach or any improvements or simplifications. And well, I didn't plan to create such a "wall of text", but hopefully it's descriptive enough. If not, we could talk F2F to clarify any details.

sculpt0r commented 3 years ago

Rebase

sculpt0r commented 3 years ago

@f1ames Ready for another review

I rewrite API usage to JS.

There is .env file required with at least AUTH_KEY, OWNER, REPO to work correctly.

There are two tests for setup workflow & dependency updater.
Also, after a run node index.js there are results of both tests in the console.

I decide to push files to the repository via GH API, rather than making local operations on the real directory. Currently, each file is committed as a separate commit.

sculpt0r commented 3 years ago

@f1ames

Have you checked if it will be doable to remove all but main/master branches on the beginning of a test run via GH API? I wonder if we should just do global clean up or maybe each test should clean after itself (the first one sounds much easier TBH).

I have to talk to you before review ;p I'm not sure what do you mean by clean after itself ;p

Since you gave me a lot of choices, here is a list of modifications:

sculpt0r commented 3 years ago

As we talked with @f1ames We leave commits created during testing to have an exact view on action & commit related to it. In case you forgot which changes you made and how the results look like.

In the end - all of this is made (should be! ;p) on testing repo.

sculpt0r commented 3 years ago

Ready for review.

sculpt0r commented 3 years ago

@f1ames - ready for review I made some ground rework, but also correct all of your comments. However, I have one thing to discuss with you ;)

I made a ground rework about preparing fixtures to test. Now they are separated into folders. Also added one config field, which is workflow and it has to match workflow yml config file. This file will be automatically added to the files list. Also, all additional files can be placed in the fixture directory. In the config file, the path to those files has to be related just to this fixture directory. Because of that, I correct some entries in README.

Also, I added three test cases, which are mentioned here. Those are not all of them, but:

Probably the next step can introduce custom GH API calls in fixtures. So we have to extend the test fixture to not only contain some configuration but also to be able to execute some code. This could be another story, based on our previous talks.

sculpt0r commented 3 years ago

What I meant in my previous #15 (review), was to have fixtures/ directory for additional files necessary for test runs, but not sure if it was clear enough, sorry see_no_evil

So I would be for simplifying it slightly:

Group tests for single workflow in a single file. Inline config into test declaration. If any extra files are needed, put it into assets/ dir.

I want to avoid 'all additional files' in one directory. But since there is not much of them. We can make it happen.

Inlining config into test declaration requires some additional changes, however, it looks more consistent and easier to maintain. So I'll rewrite it.

sculpt0r commented 3 years ago

@f1ames - ready for review

There is a request error visible if we try to get SHA for a non-existing file, but everything is working fine. This case is covered. However, I don't want to suppress errors just in case...

sculpt0r commented 3 years ago

When you run tests against empty repo it throws an error (but then proceeds anyway)

From what I see the error is because there is no setup-workflows.yml file there. Why it checks it and reports this as an error? Since tests pushes this file I would assume it usually won't be there thinking

I mention that partially here: https://github.com/ckeditor/ckeditor4-workflows-common/pull/15#issuecomment-824042833

GH Client throws an exception for 404 (which in this case is not an error, but useful information). Because there is no file on the repo, we don't want to extract SHA from that old file. We need to make this request - even if the URL is invalid for a non-existing file.

Previously I decided to try...catch any error right in the client.js module - to not spread them on the entire application. Request results are still returned, but also logged at the screen just in case. A similar 404 error could mean that someone provides the wrong GH rest API URL. But you can always verify your results via status. I don't want to try...catch each call in the application. So this log could be removed. Verifying status seems to be less code mess than wrapping everything in try...catch :thinking: WDYT?

sculpt0r commented 3 years ago

Issues to extract:

sculpt0r commented 3 years ago

@f1ames - ready for review

f1ames commented 3 years ago

Rebased onto latest master.