search

balena-io-modules / scrutinizer

Extract a git repository's metadata relying on open source conventions
Apache License 2.0
8 stars 0 forks source link

dependencies: Upgrade @octokit/rest #21

Closed dimitrisnl closed 5 years ago

dimitrisnl commented 5 years ago

Update the deprecated parts and generate package.lock

Connects-to: #20 Change-type: minor Signed-off-by: Dimitrios Lytras dimitrios@balena.io

dimitrisnl commented 5 years ago

Travis fails with 

message: 'API rate limit exceeded for 35.193.7.13. (But here\'s the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)',

checking to see if it's a bug with how we initialize the GH client.

edit: FIXED

The authentication process has changed a bit https://github.com/octokit/rest.js#authentication

dimitrisnl commented 5 years ago

@jviotti We should reconsider our test cases

For example, the integrations test isn't depending on the reference commit. We have to always be reactive to these changes, otherwise, the Scrutinizer tests will fail with false positives.

  e2e › remote: versionist (8d26949be685c9427bc6c48893572a2d5b8fe974)
  /home/travis/build/balena-io-modules/scrutinizer/test/e2e.spec.js:50
   49:     }).then((data) => {                    
   50:       test.deepEqual(data, testCase.result)
   51:     })                                     
  Difference:
    {
      architecture: null,
      contributing: null,
      dependencies: Array [ … ],
      integrations: [
  -     'ResinCI/check-commits/commits',
  +     'Versionist',
  -     'ResinCI/check-files/license',
  +     'appveyor',
  -     'ResinCI/check-files/readme',
  +     'travis-ci',
  -     'ResinCI/store-github/store-github',
  +     'AutoMerges',
  -     'ResinCI/check-versionist/versionist',
  +     'Reviewers',
  -     'ResinCI/npm/store-npm',
      ],
      license: 'Apache-2.0',
      public: true,
    }

These integrations (appveyor, etc) were valid perfectly fine at the time of the 8d26949be685c9427bc6c48893572a2d5b8fe974 commit, but this.github.repos.getProtectedBranchRequiredStatusChecks returns the current active integrations.

jviotti commented 5 years ago

@dimitrisnl That's a good point. I'm happy to record the commit the tests were made on and use that when querying the GitHub API if possible. Otherwise let me know and we can discuss another solution.

dimitrisnl commented 5 years ago

@jviotti What do you mean by recording, isn't this what we do right now?

Considering GH will respond with the active integrations at the time of the call any values we persist are prone to be outdated.

Wouldn't having a simple type checking test suffice?

dimitrisnl commented 5 years ago

This is extended to other plugins as well. # of commits, top contributors etc

The endpoints that accept ref param are limited. We can't always query for the state of the repo at a certain point in history.

jviotti commented 5 years ago

@dimitrisnl Right, but then we should make sure we test real things on real repos, otherwise its hard to prove that it really works. What about creating a test repo with various things on it, and then mark it as "archived" on GH, which will make it completely read-only on GitHub, and then we assert on that?

dimitrisnl commented 5 years ago

status: Do not merge. I'll follow Juan's suggestion and update the tests cases.

dimitrisnl commented 5 years ago

This is a bit weird. The tests fail because the array items are in a different order...

     integrations: [
        'concourse-ci/electron',
        'ResinCI/store-github/store-github',
        'ResinCI/check-versionist/versionist',
  -     'ResinCI/check-files/readme',
  +     'ResinCI/check-commits/commits',
  -     'ResinCI/check-commits/commits',
  +     'ResinCI/check-files/readme',
        'ResinCI/check-files/license',
      ],

cc @jviotti

Will sort them in both places in alphabetical order, but there must be a way to handle this with AVA

dimitrisnl commented 5 years ago

@resin-ci retest

dimitrisnl commented 5 years ago

@resin-ci retest

dimitrisnl commented 5 years ago

I've noticed that sometimes the ResinCI/check-files/license isn't present in the checks. Which causes the DeepEqual check to fail.

dimitrisnl commented 5 years ago

@resin-ci retest

dimitrisnl commented 5 years ago

@resin-ci retest

dimitrisnl commented 5 years ago

@resin-ci retest

nazrhom commented 5 years ago

@resin-ci retest

nazrhom commented 5 years ago

@resin-ci retest