Skip to content

Conversation

deusanyjunior
Copy link
Contributor

Currently the method is returning Object reference not set to an instance of an object when we try to run a batch without commands. This PR returns SqlBatchCommand list has not been initialized instead.

I'd appreciate a CI & Test run.

@deusanyjunior
Copy link
Contributor Author

@dotnet-policy-service agree company="VTEX"

@deusanyjunior deusanyjunior marked this pull request as ready for review April 29, 2025 14:31
@benrr101
Copy link
Contributor

It looks like a good idea - the change is pretty simple. However, we want to make sure that these get caught by our localization process (and that we have consistent error messaging). Could you put the error message in our /resources/string.resx file and reference that when throwing the exception, please?

@paulmedynski
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Contributor

@paulmedynski paulmedynski left a comment

Choose a reason for hiding this comment

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

Use string resources to support localization.

@benrr101 benrr101 added this to the 6.1-preview2 milestone Apr 30, 2025
@paulmedynski
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Contributor

@benrr101 benrr101 left a comment

Choose a reason for hiding this comment

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

Found the test failure! One character change, should be easy fix. :)

{
get
{
return ResourceManager.GetString("ADP_NoSqlBatchCommandList", resourceCulture);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks real good except ... Either this line should be ADP_NoSqlBatchCommandsList (with an s) OR the resx file should be ADP_NoSqlBatchCommandList (without an s). I personally would vote for the second choice

As it stands right now, the mismatch is causing the unit test to fail due to ArgumentNullException coming from looking up the resx.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Great suggestion. Just fixed.

@benrr101
Copy link
Contributor

benrr101 commented May 7, 2025

/azp run

Copy link
Contributor

@benrr101 benrr101 left a comment

Choose a reason for hiding this comment

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

If the build passes, I'm on board with this change 🚢

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@deusanyjunior
Copy link
Contributor Author

@benrr101 Do you have any clue about tests? It seems the results are not good. =\

@paulmedynski
Copy link
Contributor

Looks like some network blips. I'm re-running the failed tests.

@paulmedynski paulmedynski merged commit e5cef80 into dotnet:main May 8, 2025
237 checks passed
Copy link

codecov bot commented May 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (a11dae7) to head (f8cc913).
Report is 16 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #3310       +/-   ##
==========================================
- Coverage   72.94%       0   -72.95%     
==========================================
  Files         298       0      -298     
  Lines       57002       0    -57002     
==========================================
- Hits        41579       0    -41579     
+ Misses      15423       0    -15423     
Flag Coverage Δ
addons ?
netcore ?
netfx ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants