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

Prevent NoMethodError in group update creation #9366

Merged
merged 2 commits into from Mar 26, 2024
Merged

Conversation

bdragon
Copy link
Member

@bdragon bdragon commented Mar 26, 2024

We are seeing runtime exceptions due to calling a method on an argument that may be nil. This change properly handles nil arguments.

@bdragon bdragon added T: bug 🐞 Something isn't working F: grouped-updates 🎳 Relates to bumping more than one dependency in a single PR labels Mar 26, 2024
@bdragon bdragon requested a review from a team as a code owner March 26, 2024 04:08
Copy link
Member

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

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

Code change looks good, although I wonder how hard it'd be to add a unit test ensuring no future regressions?

@bdragon
Copy link
Member Author

bdragon commented Mar 26, 2024

I wonder how hard it'd be to add a unit test ensuring no future regressions?

Dependabot::Updater::GroupUpdateCreation is a module and it looks like it doesn't have any tests. I'm guessing it wasn't really intended to be treated as a public API, the idea was probably that the functionality would be covered by more e2e-like updater tests or by tests for the classes it gets included in. It's included in three classes and one of those classes has tests, so I added a test there.

@bdragon bdragon merged commit 5d769a9 into main Mar 26, 2024
140 checks passed
@bdragon bdragon deleted the bdragon/fix-nil-err branch March 26, 2024 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F: grouped-updates 🎳 Relates to bumping more than one dependency in a single PR T: bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants