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

Enable RSpec/EmptyExampleGroup #9731

Merged
merged 4 commits into from
May 17, 2024
Merged

Conversation

robaiken
Copy link
Contributor

@robaiken robaiken requested review from a team as code owners May 14, 2024 13:28
@github-actions github-actions bot added L: php:composer Issues and code for Composer L: rust:cargo Rust crates via cargo L: dotnet:nuget NuGet packages via nuget or dotnet labels May 14, 2024
@robaiken
Copy link
Contributor Author

Do we want to keep these tests or are we happy with deleting them?

@jurre
Copy link
Member

jurre commented May 14, 2024

@robaiken could you dig out why they were disabled? As it stands, I'd rather delete them then keep them commented out, but if it's easy to fix them and make them reliable/fast, that seems better?

@@ -691,31 +691,6 @@
end
end

context "when there are patches (composer v2)" do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

patch data isn't going to be re-applied to composer.lock

#6620

@robaiken
Copy link
Contributor Author

@jurre I couldn't find the context about why the NuGet tests were skipped. Both tests seemed to have issues related to VCR

# to match_array(%w(NuGet.Config packages.config))
# end
it "fetches the packages.config" do
skip "This test was commented out and does not work at the moment"
Copy link
Member

Choose a reason for hiding this comment

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

Do we expect this will realistically ever be fixed?

# expect(subject[:version]).to eq(version_class.new("6.5.0"))
# end
it "returns the expected version" do
skip "This test was commented out and does not work at the moment"
Copy link
Member

Choose a reason for hiding this comment

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

Same here, is there are plan to address this, or will this realistically just remain skipped?

Copy link
Member

@jurre jurre left a comment

Choose a reason for hiding this comment

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

Looks good, but I would personally rather remove the skipped tests if there is no plan to fix them any time soon. Consider that just an opinion, not a request for change.

@robaiken
Copy link
Contributor Author

@jurre I think it's up to the Azure team if they want to keep the tests or not. I have sent them a message, so they are aware of the skipped tests.

@robaiken robaiken self-assigned this May 15, 2024
@robaiken robaiken enabled auto-merge (squash) May 17, 2024 11:43
@robaiken robaiken merged commit dbd56f9 into main May 17, 2024
71 checks passed
@robaiken robaiken deleted the robaiken/enable-RSpec-EmptyExampleGroup branch May 17, 2024 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: dotnet:nuget NuGet packages via nuget or dotnet L: php:composer Issues and code for Composer L: rust:cargo Rust crates via cargo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants