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. |
|
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
…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.
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.
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.
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.
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:
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.