Skip to content

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

Merged
Zakaria-Kofiro merged 1 commit intomasterfrom
zkofiro/code-cov-scope-fix
Feb 16, 2023
Merged

Code Coverage Follow up: Updating test variable scope to private #233
Zakaria-Kofiro merged 1 commit intomasterfrom
zkofiro/code-cov-scope-fix

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
@Test
public void testCommandListenerTest_1() {
CommandListener.main(new String[]{});
CommandListener.startTest();
Copy link
Contributor

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
Contributor

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


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

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
Contributor

@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 force-pushed the zkofiro/code-cov-scope-fix branch from 2db7da8 to 4d05244 Compare February 16, 2023 20:58
@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.

2 participants