tests that require GitHub token are always skipped in CI due to silly typo in GitHub Actions workflow

[boegel]

There's a silly typo in the GitHub Actions workflow file we use to run the unit tests (.github/workflows/unit_tests.yml):

easybuild-framework/.github/workflows/unit_tests.yml

Line 130 in 89fc160

if [[ ! "${{matrix.modules_tool}}" =~ 'Lmod-7' ]] && [[ ! "${{matrix.modules_tool}}" =~ 'modules-' ]] && [[ "${{matrix.modules_syntax}}" == 'Lua' ]]; then

${{matrix.modules_syntax}} should be ${{matrix.module_syntax}}

As a result of this, the GitHub token that is available when the tests are being run are never run, even if a GitHub token is available (which is the case when the tests are run after a PR is merged into develop), and we skip any tests that require a GitHub token if none is available.

This problems was introduced in #3938 (merged Jan'22)

I looked into fixing this, but then I'm seeing trouble with the permissions of GitHub token that's available when the tests are run, see for example https://github.com/boegel/easybuild-framework/actions/runs/4182110823/jobs/7244807091

Relevant part of output of eb --check-github:

* GitHub user...easybuild_test => OK
* GitHub token...ghp..FGM (len: 40) => FAIL (validation failed)

cc @migueldiascosta

[boegel]

When I ran the github tests myself (with a GitHub token available), I see two failures currently:

• test_github_reasons_for_closing

$ python3 -O -m test.framework.github test_github_reasons_for_closing
Filtered GithubTest tests using 'test_github_reasons_for_closing', retained 1/30 tests: test_github_reasons_for_closing
F
======================================================================
FAIL: test_github_reasons_for_closing (__main__.GithubTest)
Test reasons_for_closing function.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Volumes/work/easybuild-framework/test/framework/github.py", line 289, in test_github_reasons_for_closing
    self.assertIn(pattern, stdout)
AssertionError: 'Status of last commit is SUCCESS' not found in "Last comment on 2018-07-05 at 02:01:53, by migueldiascosta, was:\n\n@hcartiaux, this PR is being closed for the following reason(s): no activity for > 6 months, obsoleted by more recent PRs.\nPlease don't hesitate to reopen this PR or add a comment if you feel this contribution is still relevant.\nFor more information on our policy w.r.t. closing PRs, see https://easybuild.readthedocs.io/en/latest/Contributing.html#why-a-pull-request-may-be-closed-by-a-maintainer\nNo activity since 2018-07-05 at 02:01:54\n* QEMU-2.4.0\n"

----------------------------------------------------------------------
Ran 1 test in 9.686s

FAILED (failures=1)

• test_github_det_commit_status

$ python3 -O -m test.framework.github test_github_det_commit_status
Filtered GithubTest tests using 'test_github_det_commit_status', retained 1/30 tests: test_github_det_commit_status
F
======================================================================
FAIL: test_github_det_commit_status (__main__.GithubTest)
Test det_commit_status function.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Volumes/work/easybuild-framework/easybuild/base/testing.py", line 97, in assertEqual
    super(TestCase, self).assertEqual(a, b)
  File "/Library/Developer/CommandLineTools/Library/Frameworks/Python3.framework/Versions/3.9/lib/python3.9/unittest/case.py", line 831, in assertEqual
    assertion_func(first, second, msg=msg)
AssertionError: None != 'success'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Volumes/work/easybuild-framework/test/framework/github.py", line 639, in test_github_det_commit_status
    self.assertEqual(res, 'success')
  File "/Volumes/work/easybuild-framework/easybuild/base/testing.py", line 119, in assertEqual
    raise AssertionError("%s:\nDIFF%s:\n%s" % (msg, limit, ''.join(diff[:self.ASSERT_MAX_DIFF])))
AssertionError: None != 'success':
DIFF:
- None

----------------------------------------------------------------------
Ran 1 test in 0.763s

There's also a failing GitHub-related test in the options test suite:

$ python3 -O -m test.framework.options test_github_merge_pr
Filtered CommandLineOptionsTest tests using 'test_github_merge_pr', retained 1/136 tests: test_github_merge_pr
F
======================================================================
FAIL: test_github_merge_pr (__main__.CommandLineOptionsTest)
Test use of --merge-pr (dry run)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Volumes/work/easybuild-framework/easybuild/base/testing.py", line 97, in assertEqual
    super(TestCase, self).assertEqual(a, b)
  File "/Library/Developer/CommandLineTools/Library/Frameworks/Python3.framework/Versions/3.9/lib/python3.9/unittest/case.py", line 831, in assertEqual
    assertion_func(first, second, msg=msg)
AssertionError: "* ta[77 chars]!\n* test suite passes: (status: None) => not [165 chars]way)" != "* ta[77 chars]!\n* approved review: MISSING => not eligible [99 chars]way)"
  * targets some_branch branch: FAILED; found 'develop' => not eligible for merging!
- * test suite passes: (status: None) => not eligible for merging!
  * approved review: MISSING => not eligible for merging!

  WARNING: Review indicates this PR should not be merged (use -f/--force to do so anyway)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Volumes/work/easybuild-framework/test/framework/options.py", line 4766, in test_github_merge_pr
    self.assertEqual(stderr.strip(), expected_stderr)
  File "/Volumes/work/easybuild-framework/easybuild/base/testing.py", line 119, in assertEqual
    raise AssertionError("%s:\nDIFF%s:\n%s" % (msg, limit, ''.join(diff[:self.ASSERT_MAX_DIFF])))
AssertionError: "* ta[77 chars]!\n* test suite passes: (status: None) => not [165 chars]way)" != "* ta[77 chars]!\n* approved review: MISSING => not eligible [99 chars]way)"
  * targets some_branch branch: FAILED; found 'develop' => not eligible for merging!
- * test suite passes: (status: None) => not eligible for merging!
  * approved review: MISSING => not eligible for merging!

  WARNING: Review indicates this PR should not be merged (use -f/--force to do so anyway):
DIFF:
  * targets some_branch branch: FAILED; found 'develop' => not eligible for merging!
- * test suite passes: (status: None) => not eligible for merging!
  * approved review: MISSING => not eligible for merging!



----------------------------------------------------------------------
Ran 1 test in 6.940s

FAILED (failures=1)

[migueldiascosta]

hm, is this related to #3405 (comment)?

[migueldiascosta]

I think the problem is indeed the changes in det_commit_status and the fact that we're no longer checking with Travis...

I don't know how to fix that, but just to make clear exactly what's failing now, these changes make all the tests pass:

diff --git a/test/framework/github.py b/test/framework/github.py
index 8802aaf..be30407 100644
--- a/test/framework/github.py
+++ b/test/framework/github.py
@@ -280,7 +280,6 @@ class GithubTest(EnhancedTestCase):
         self.assertTrue(isinstance(res, list))
         self.assertEqual(stderr.strip(), "WARNING: Using easyconfigs from closed PR #1844")
         patterns = [
-            "Status of last commit is SUCCESS",
             "Last comment on",
             "No activity since",
             "* QEMU-2.4.0",
@@ -636,37 +635,37 @@ class GithubTest(EnhancedTestCase):
         # ancient commit, from Jenkins era
         commit_sha = 'ec5d6f7191676a86a18404616691796a352c5f1d'
         res = gh.det_commit_status('easybuilders', 'easybuild-easyconfigs', commit_sha, GITHUB_TEST_ACCOUNT)
-        self.assertEqual(res, 'success')
+        self.assertEqual(res, None)
 
         # commit with failing tests from Travis CI era (no GitHub Actions yet)
         commit_sha = 'd0c62556caaa78944722dc84bbb1072bf9688f74'
         res = gh.det_commit_status('easybuilders', 'easybuild-easyconfigs', commit_sha, GITHUB_TEST_ACCOUNT)
-        self.assertEqual(res, 'failure')
+        self.assertEqual(res, None)
 
         # commit with passing tests from Travis CI era (no GitHub Actions yet)
         commit_sha = '21354990e4e6b4ca169b93d563091db4c6b2693e'
         res = gh.det_commit_status('easybuilders', 'easybuild-easyconfigs', commit_sha, GITHUB_TEST_ACCOUNT)
-        self.assertEqual(res, 'success')
+        self.assertEqual(res, None)
 
         # commit with failing tests, tested by both Travis CI and GitHub Actions
         commit_sha = '3a596de93dd95b651b0d1503562d888409364a96'
         res = gh.det_commit_status('easybuilders', 'easybuild-easyconfigs', commit_sha, GITHUB_TEST_ACCOUNT)
-        self.assertEqual(res, 'failure')
+        self.assertEqual(res, None)
 
         # commit with passing tests, tested by both Travis CI and GitHub Actions
         commit_sha = '1fba8ac835d62e78cdc7988b08f4409a1570cef1'
         res = gh.det_commit_status('easybuilders', 'easybuild-easyconfigs', commit_sha, GITHUB_TEST_ACCOUNT)
-        self.assertEqual(res, 'success')
+        self.assertEqual(res, None)
 
         # commit with failing tests, only tested by GitHub Actions
         commit_sha = 'd7130683f02fe8284df3557f0b2fd3947c2ea153'
         res = gh.det_commit_status('easybuilders', 'easybuild-easyconfigs', commit_sha, GITHUB_TEST_ACCOUNT)
-        self.assertEqual(res, 'failure')
+        self.assertEqual(res, None)
 
         # commit with passing tests, only tested by GitHub Actions
         commit_sha = 'e6df09700a1b90c63b4f760eda4b590ee1a9c2fd'
         res = gh.det_commit_status('easybuilders', 'easybuild-easyconfigs', commit_sha, GITHUB_TEST_ACCOUNT)
-        self.assertEqual(res, 'success')
+        self.assertEqual(res, None)
 
         # commit in test repo where no CI is running at all
         commit_sha = '8456f867b03aa001fd5a6fe5a0c4300145c065dc'
diff --git a/test/framework/options.py b/test/framework/options.py
index e00c827..a58ed7a 100644
--- a/test/framework/options.py
+++ b/test/framework/options.py
@@ -4693,7 +4693,6 @@ class CommandLineOptionsTest(EnhancedTestCase):
 
         expected_stdout = '\n'.join([
             "Checking eligibility of easybuilders/easybuild-easyconfigs PR #4781 for merging...",
-            "* test suite passes: OK",
             "* last test report is successful: OK",
             "* no pending change requests: OK",
             "* milestone is set: OK (3.3.1)",
@@ -4701,6 +4700,7 @@ class CommandLineOptionsTest(EnhancedTestCase):
         ])
         expected_stderr = '\n'.join([
             "* targets some_branch branch: FAILED; found 'develop' => not eligible for merging!",
+            "* test suite passes: (status: None) => not eligible for merging!",
             "* approved review: MISSING => not eligible for merging!",
             '',
             "WARNING: Review indicates this PR should not be merged (use -f/--force to do so anyway)",
@@ -4717,19 +4717,17 @@ class CommandLineOptionsTest(EnhancedTestCase):
         expected_stdout = '\n'.join([
             "Checking eligibility of easybuilders/easybuild-easyconfigs PR #4832 for merging...",
             "* targets develop branch: OK",
-            "* test suite passes: OK",
             "* last test report is successful: OK",
             "* no pending change requests: OK",
             "* approved review: OK (by wpoely86)",
             "* milestone is set: OK (3.3.1)",
             "* mergeable state is clean: PR is already merged",
+        ])
+        expected_stderr = '\n'.join([
+            "* test suite passes: (status: None) => not eligible for merging!\n"
             '',
-            "Review OK, merging pull request!",
-            '',
-            "[DRY RUN] Adding comment to easybuild-easyconfigs issue #4832: 'Going in, thanks @boegel!'",
-            "[DRY RUN] Merged easybuilders/easybuild-easyconfigs pull request #4832",
+            "WARNING: Review indicates this PR should not be merged (use -f/--force to do so anyway)",
         ])
-        expected_stderr = ''
         self.assertEqual(stderr.strip(), expected_stderr)
         self.assertTrue(stdout.strip().endswith(expected_stdout), "'%s' ends with '%s'" % (stdout, expected_stdout))
 
@@ -4742,17 +4740,19 @@ class CommandLineOptionsTest(EnhancedTestCase):
             '--github-user=%s' % GITHUB_TEST_ACCOUNT,
         ]
         stdout, stderr = self._run_mock_eb(args, do_build=True, raise_error=True, testing=False)
-        self.assertEqual(stderr.strip(), '')
+        expected_stderr = '\n'.join([
+            "* test suite passes: (status: None) => not eligible for merging!\n"
+            '',
+            "WARNING: Review indicates this PR should not be merged (use -f/--force to do so anyway)",
+        ])
+        self.assertEqual(stderr.strip(), expected_stderr)
         expected_stdout = '\n'.join([
             "Checking eligibility of easybuilders/easybuild-easyblocks PR #1206 for merging...",
             "* targets develop branch: OK",
-            "* test suite passes: OK",
             "* no pending change requests: OK",
             "* approved review: OK (by migueldiascosta)",
             "* milestone is set: OK (3.3.1)",
             "* mergeable state is clean: PR is already merged",
-            '',
-            "Review OK, merging pull request!",
         ])
         self.assertIn(expected_stdout, stdout)

[boegel]

I think the problem is indeed the changes in det_commit_status and the fact that we're no longer checking with Travis...

I don't know how to fix that, but just to make clear exactly what's failing now, these changes make all the tests pass:

We'll need to basically re-implement the tests that rely on det_commit_status with more recent commits/PRs that do have a proper status set, I see no other way to properly fix this...

[boegel]

Here's what I get now as list of commit statuses (empty list) for the ancient commit easybuilders/easybuild-easyconfigs@ec5d6f7 (first commit used in test_github_det_commit_status):

{'commit_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/commits/ec5d6f7191676a86a18404616691796a352c5f1d',
 'repository': {'archive_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/{archive_format}{/ref}',
                'assignees_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/assignees{/user}',
                'blobs_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/git/blobs{/sha}',
                'branches_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/branches{/branch}',
                'collaborators_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/collaborators{/collaborator}',
                'comments_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/comments{/number}',
                'commits_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/commits{/sha}',
                'compare_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/compare/{base}...{head}',
                'contents_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/contents/{+path}',
                'contributors_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/contributors',
                'deployments_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/deployments',
                'description': 'A collection of easyconfig files that describe '
                               'which software to build using which build '
                               'options with EasyBuild.',
                'downloads_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/downloads',
                'events_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/events',
                'fork': False,
                'forks_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/forks',
                'full_name': 'easybuilders/easybuild-easyconfigs',
                'git_commits_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/git/commits{/sha}',
                'git_refs_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/git/refs{/sha}',
                'git_tags_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/git/tags{/sha}',
                'hooks_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/hooks',
                'html_url': 'https://github.com/easybuilders/easybuild-easyconfigs',
                'id': 6228403,
                'issue_comment_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/issues/comments{/number}',
                'issue_events_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/issues/events{/number}',
                'issues_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/issues{/number}',
                'keys_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/keys{/key_id}',
                'labels_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/labels{/name}',
                'languages_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/languages',
                'merges_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/merges',
                'milestones_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/milestones{/number}',
                'name': 'easybuild-easyconfigs',
                'node_id': 'MDEwOlJlcG9zaXRvcnk2MjI4NDAz',
                'notifications_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/notifications{?since,all,participating}',
                'owner': {'avatar_url': 'https://avatars.githubusercontent.com/u/29568382?v=4',
                          'events_url': 'https://api.github.com/users/easybuilders/events{/privacy}',
                          'followers_url': 'https://api.github.com/users/easybuilders/followers',
                          'following_url': 'https://api.github.com/users/easybuilders/following{/other_user}',
                          'gists_url': 'https://api.github.com/users/easybuilders/gists{/gist_id}',
                          'gravatar_id': '',
                          'html_url': 'https://github.com/easybuilders',
                          'id': 29568382,
                          'login': 'easybuilders',
                          'node_id': 'MDEyOk9yZ2FuaXphdGlvbjI5NTY4Mzgy',
                          'organizations_url': 'https://api.github.com/users/easybuilders/orgs',
                          'received_events_url': 'https://api.github.com/users/easybuilders/received_events',
                          'repos_url': 'https://api.github.com/users/easybuilders/repos',
                          'site_admin': False,
                          'starred_url': 'https://api.github.com/users/easybuilders/starred{/owner}{/repo}',
                          'subscriptions_url': 'https://api.github.com/users/easybuilders/subscriptions',
                          'type': 'Organization',
                          'url': 'https://api.github.com/users/easybuilders'},
                'private': False,
                'pulls_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/pulls{/number}',
                'releases_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/releases{/id}',
                'stargazers_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/stargazers',
                'statuses_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/statuses/{sha}',
                'subscribers_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/subscribers',
                'subscription_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/subscription',
                'tags_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/tags',
                'teams_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/teams',
                'trees_url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/git/trees{/sha}',
                'url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs'},
 'sha': 'ec5d6f7191676a86a18404616691796a352c5f1d',
 'state': 'pending',
 'statuses': [],
 'total_count': 0,
 'url': 'https://api.github.com/repos/easybuilders/easybuild-easyconfigs/commits/ec5d6f7191676a86a18404616691796a352c5f1d/status'}

For check suites (basically what's used for the tests being run in GitHub Actions):

{'check_suites': [], 'total_count': 0}

(makes sense, because that commit pre-dates the testing we're doing now in GitHub Actions)

There's not much that det_commit_status can work with there, so None is a logical result for old commits like this.