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

Code Coverage Follow up: Updating test variable scope to private #233

Merged
merged 1 commit into from
Feb 16, 2023

Conversation

Zakaria-Kofiro
Copy link
Collaborator

title: - Test variable scope changed to private across new tests + fixed a test exiting when ran in IDE (code cov unchanged)

Please make sure these check boxes are checked before submitting

  • ** Squashed Commits **
  • ** All Tests Passed ** - mvn clean test -P default

** PR review process **

  • Requires one +1 from a reviewer
  • Repository owners will merge your PR once it is approved.

@shawn-h-park shawn-h-park self-requested a review February 16, 2023 19:33
@@ -6,6 +6,5 @@ public class CommandListenerTest {
@Test
public void testCommandListenerTest_1() {
CommandListener.main(new String[]{});
CommandListener.startTest();
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Zakaria-Kofiro Not exactly related to your goal of scoping test vars to private, but wondering what testCommandListenerTest_1 is testing for with the absence of any assertions

@BeforeEach
public void init() {
instance = APITestHarness.getInstance();
APITestHarness instance = APITestHarness.getInstance();
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Zakaria-Kofiro Since this var instance of APITestHarness is not being used within the test can we remove the variable? Also because it's a private static var within APITestHarness.Java

@@ -9,8 +9,8 @@

public class OidcSsoConfigTest {

HierarchicalConfiguration config = new HierarchicalConfiguration();
OidcSsoConfig oidc = null;
private final HierarchicalConfiguration config = new HierarchicalConfiguration();
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Zakaria-Kofiro Not related to this change, but can we remove unused import com.intuit.tank.vm.vmManager.VMInstanceRequest on line 3

Copy link
Collaborator

@shawn-h-park shawn-h-park left a comment

Choose a reason for hiding this comment

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

Lgtm

fix: removed unneeded var and import, added assert
@Zakaria-Kofiro Zakaria-Kofiro merged commit 0593cc4 into master Feb 16, 2023
@Zakaria-Kofiro Zakaria-Kofiro deleted the zkofiro/code-cov-scope-fix branch February 16, 2023 20:59
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.

None yet

2 participants