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
Stop hiding exception raised by composer #9657
Conversation
I was surprised to see you went after this by raising rather than logging the error... Did you see my comment in #6290 (comment) ? I explained it more in #6289 (comment)... basically the problem is never going to fully go away, so if we log, then we solve for exposing the error to users, but without blowing up the job--ie, we at least do a partial update for them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach makes sense to me, just two questions/suggestions about the implementation.
The skeleton of where you're going on this looks great! 🎉 Just need a few details tweaked and then this should be good to go... |
use a fixture file based on a real package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
…error' into robaiken/dont-suppress-composer-error
it { is_expected.to be_nil } | ||
|
||
it "logs an error" do | ||
allow(Dependabot.logger).to receive(:error) | ||
Dependabot.logger.error | ||
expect(Dependabot.logger).to have_received(:error).once | ||
context "logs an error" do | ||
before { allow(Dependabot.logger).to receive(:error) } | ||
|
||
it do | ||
is_expected.to be_nil | ||
expect(Dependabot.logger).to have_received(:error).at_least(:twice) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why you broke this out as a separate section? Why not blend this with the existing test in line 529?
The spacing of this suggestion is off, so please don't take it verbatim, but this illustrates the general concept...
it { is_expected.to be_nil } | |
it "logs an error" do | |
allow(Dependabot.logger).to receive(:error) | |
Dependabot.logger.error | |
expect(Dependabot.logger).to have_received(:error).once | |
context "logs an error" do | |
before { allow(Dependabot.logger).to receive(:error) } | |
it do | |
is_expected.to be_nil | |
expect(Dependabot.logger).to have_received(:error).at_least(:twice) | |
end | |
before { allow(Dependabot.logger).to receive(:error) } | |
it { is_expected.to be_nil } | |
expect(Dependabot.logger).to have_received(:error).once |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to work as it fails to run the underlining code before it runs expect(Dependabot.logger).to have_received(:error).once
. I'm not sure why this happens, I tried using receives
instead but I was still failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it stand now, this text fixture is unused because the test that you added that used it was deleted... you'll want to either re-add that test or delete this fixture... my vote is for re-adding that test as I think it's important to communicate to the develeoper/test the scenario where Dependabot can't do anything because the native compiled extension is missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The handle_composer_errors
has a condition that checks for missing Native Extensions in the composer error message. If there is a missing Native extension, then it will be caught by this check:
dependabot-core/composer/lib/dependabot/composer/update_checker/version_resolver.rb
Lines 266 to 275 in 78cf509
elsif error.message.match?(MISSING_EXPLICIT_PLATFORM_REQ_REGEX) | |
# These errors occur when platform requirements declared explicitly | |
# in the composer.json aren't met. | |
missing_extensions = | |
error.message.scan(MISSING_EXPLICIT_PLATFORM_REQ_REGEX) | |
.map do |extension_string| | |
name, requirement = extension_string.strip.split(" ", 2) | |
{ name: name, requirement: requirement } | |
end | |
raise MissingExtensions, missing_extensions |
So it shouldn't be possible for this to happen
Previously, errors from Composer were suppressed, which made it difficult for users to understand what caused the issue. With this change, we will now display all the errors without hiding them. This allows users to identify and investigate potential problems more easily.