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

Add smoke tests for grouped updates #62

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Conversation

bdragon
Copy link
Member

@bdragon bdragon commented Apr 25, 2023

No description provided.

@bdragon bdragon force-pushed the bdragon/smoke-grouped branch 2 times, most recently from 322ed0e to f3ecb60 Compare April 26, 2023 17:31
@bdragon bdragon force-pushed the bdragon/smoke-grouped branch 2 times, most recently from 6713ede to ea0e96c Compare April 28, 2023 19:10
source:
provider: github
repo: dependabot/smoke-tests
directory: /npm
Copy link
Member

@Nishnha Nishnha May 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
directory: /npm
directory: /npm
branch: bdragon/smoke-grouped

Since this job uses dependencies that are newly added to this branch, we also need to set the branch in the job definition.

With the branch set we see PRs created for each group:

+------------------------------------------------------------------------------------------------------------------------------------+
|                                                Changes to Dependabot Pull Requests                                                 |
+---------+--------------------------------------------------------------------------------------------------------------------------+
| created | fetch-factory ( from 0.0.1 to 0.2.1 )                                                                                    |
| created | babel-jest ( from 28.1.1 to 29.5.0 ), @babel/compat-data ( from 7.21.4 to 7.21.7 ), @babel/generator ( from 7.21.4 to... |
| created | babel-jest ( from 28.1.1 to 29.5.0 ), diff-sequences ( from 28.1.1 to 29.4.3 ), eslint-plugin-jest ( from 26.5.3 to 2... |
| created | eslint-plugin-jest ( from 26.5.3 to 27.2.1 )                                                                             |
| created | @types/node ( from 18.16.2 to 18.16.3 )                                                                                  |
| created | caniuse-lite ( from 1.0.30001481 to 1.0.30001482 )                                                                       |
| created | electron-to-chromium ( from 1.4.377 to 1.4.382 )                                                                         |
+---------+--------------------------------------------------------------------------------------------------------------------------+
Dependabot encountered '7' error(s) during execution, please check the logs for more details.
+------------------------------------------------------+
|            Dependencies failed to update             |
+--------------------------------------+---------------+
| babel-plugin-polyfill-corejs3        | unknown_error |
| @typescript-eslint/scope-manager     | unknown_error |
| @typescript-eslint/types             | unknown_error |
| @typescript-eslint/typescript-estree | unknown_error |
| @typescript-eslint/utils             | unknown_error |
| @typescript-eslint/visitor-keys      | unknown_error |
| @typescript-eslint/type-utils        | unknown_error |
+--------------------------------------+---------------+

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why the errors show up as unknown_error in the summary. During the job run they are presented as a Dependabot::NpmAndYarn::FileUpdater::NoChangeError which I think is because the dependencies are updated to a newer version but because they're transitive, the dependency files we pass in are not modified?

Copy link
Contributor

@brrygrdn brrygrdn May 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect NoChangeError is unexpected and gets reported back as unknown to the service as we generally do not expect it happen unless something is wrong with npm or the project.

If the resultant PR is correct, we might need to tolerate this error for grouped updates and step over any loops which produce it 🤔 - or do we need to extend the files we send into each loop to include extras from the project directory?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once this PR is merged into main you can remove the branch: key (either as a separate PR or directly committing to main)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a look, @Nishnha!

I added most of these dependencies to the package.json on this branch and the job definition has the sha of the head commit on the branch, so I don't think it would be working at all if it were the missing branch. Errors aside, those groups still don't look right—for example, fetch-factory should be grouped with lodash but it's not.

Copy link
Member

@Nishnha Nishnha May 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, just seeing your reply now @bdragon!

I do think the missing branch is the issue - the job definition has commit: ~ instead of the SHA as far as I can tell.
At least, the job run worked in the CLI after I included the branch name

As for the groups not looking right I also found it strange!
The CLI says that lodash was updated and I would also expect it to be grouped with fetch-factory, but I don't see it in the PR summary... maybe there's another bug at play here?

Here's a gist of my CLI run https://gist.github.com/Nishnha/cd0aab0e146dbe23d78f7b51c9d7daaa

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

Successfully merging this pull request may close these issues.

3 participants