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

Stop hiding exception raised by composer #9657

Merged
merged 22 commits into from
May 15, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
adff15b
Stop hiding exception raised by composer
robaiken May 2, 2024
5da1fe1
Log error, rather than raise it
robaiken May 3, 2024
be91073
lint fix
robaiken May 3, 2024
e3bf86d
Merge branch 'main' into robaiken/dont-suppress-composer-error
robaiken May 3, 2024
ff8c892
Merge branch 'main' into robaiken/dont-suppress-composer-error
robaiken May 3, 2024
df0af0f
Adding test for missing native extension
robaiken May 7, 2024
4925f2c
remove debugger
robaiken May 7, 2024
bf84d05
lint
robaiken May 7, 2024
f763edd
added test for missing native extension logs an error
robaiken May 8, 2024
b7ca065
lint
robaiken May 8, 2024
fc0b827
Merge branch 'main' into robaiken/dont-suppress-composer-error
robaiken May 8, 2024
667da5b
updating test descriptions
robaiken May 8, 2024
b86369b
Merge branch 'main' into robaiken/dont-suppress-composer-error
robaiken May 8, 2024
6a4ac2e
Merge branch 'main' into robaiken/dont-suppress-composer-error
robaiken May 9, 2024
312af66
updating test to check logger is called
robaiken May 10, 2024
8328573
Merge remote-tracking branch 'origin/robaiken/dont-suppress-composer-…
robaiken May 10, 2024
398e260
Removing fixture
robaiken May 14, 2024
d977648
Improving spys
robaiken May 14, 2024
78cf509
lint
robaiken May 14, 2024
500e7b5
Merge branch 'main' into robaiken/dont-suppress-composer-error
robaiken May 14, 2024
532da13
Merge branch 'main' into robaiken/dont-suppress-composer-error
robaiken May 15, 2024
2f82f30
Merge branch 'main' into robaiken/dont-suppress-composer-error
robaiken May 15, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ def handle_composer_errors(error)

# If there *is* a lockfile we can't confidently distinguish between
# cases where we can't install and cases where we can't update. For
# now, we therefore just ignore the dependency.
# now, we therefore just ignore the dependency and log the error.

Dependabot.logger.error(error.message)
robaiken marked this conversation as resolved.
Show resolved Hide resolved
error.backtrace.each { |line| Dependabot.logger.error(line) }
Expand Down
32 changes: 7 additions & 25 deletions composer/spec/dependabot/composer/update_checker_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -528,10 +528,13 @@

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

Suggested change
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

Copy link
Contributor Author

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.

end

context "and there is no lockfile" do
Expand All @@ -547,27 +550,6 @@
end
end

context "missing native extension" do
let(:dependency_name) { "phpunit/php-code-coverage" }
let(:dependency_version) { "11.0.0" }
let(:requirements) do
[{
requirement: "11.0.*",
file: "composer.json",
groups: ["runtime"],
source: { type: "path" }
}]
end

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
end
end

robaiken marked this conversation as resolved.
Show resolved Hide resolved
context "with a git source dependency" do
let(:project_name) { "git_source" }

Expand Down