You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "jdyer1 (via GitHub)" <gi...@apache.org> on 2023/11/21 17:21:47 UTC
[PR] Fix test failure on J9: do not mock jre class [solr]
jdyer1 opened a new pull request, #2086:
URL: https://github.com/apache/solr/pull/2086
[No JIRA Issue]
- Fix test failure on J9: do not mock jre class
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Fix test failure on J9: do not mock jre class [solr]
Posted by "mkhludnev (via GitHub)" <gi...@apache.org>.
mkhludnev commented on code in PR #2086:
URL: https://github.com/apache/solr/pull/2086#discussion_r1403690969
##########
solr/core/src/test/org/apache/solr/core/BlobRepositoryMockingTest.java:
##########
@@ -71,7 +71,8 @@ public void setUp() throws Exception {
super.setUp();
blobFetched = false;
blobKey = "";
- reset(mocks);
+ reset(mockContainer);
Review Comment:
Not getting your point. Please elaborate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Fix test failure on J9: do not mock jre class [solr]
Posted by "madrob (via GitHub)" <gi...@apache.org>.
madrob commented on code in PR #2086:
URL: https://github.com/apache/solr/pull/2086#discussion_r1403698891
##########
solr/core/src/test/org/apache/solr/core/BlobRepositoryMockingTest.java:
##########
@@ -71,7 +71,8 @@ public void setUp() throws Exception {
super.setUp();
blobFetched = false;
blobKey = "";
- reset(mocks);
+ reset(mockContainer);
Review Comment:
With this change, do we still use mocks field? Or is it used elsewhere and only skipped in this one test?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Fix test failure on J9: do not mock jre class [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #2086:
URL: https://github.com/apache/solr/pull/2086#issuecomment-1823596078
@jdyer1 I strongly recommend against pushing commits to main from your machine. If it were up to me, it'd be made to be impossible to help avoid mistakes. Nowadays we use GitHub which runs some code quality/style checks that would have been in a failing state. They would show on this PR screen here but are now absent because the state transitioned to closed. Once you resolved those things, they would show in green and then you could merge via a Squash-Merge button. It'd also ask for a commit message, which you should then edit. Only the branch_9x backport would you do from your machine, but then only after waiting for a passing build on main.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Fix test failure on J9: do not mock jre class [solr]
Posted by "jdyer1 (via GitHub)" <gi...@apache.org>.
jdyer1 commented on PR #2086:
URL: https://github.com/apache/solr/pull/2086#issuecomment-1821350308
It has been a _Very long time_ since I have used my commit powers here. This J9 test failure seemed like a nice way to try and get back into it a little. I didn't test this with J9 but converting the mock on the jre class seemed like an easy way to fix this. This now uses a real instance of the Map. I will try and merge this in a day if there are no objections.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Fix test failure on J9: do not mock jre class [solr]
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on PR #2086:
URL: https://github.com/apache/solr/pull/2086#issuecomment-1823499479
You left main branch failing precommit
```
Execution failed for task ':solr:core:spotlessJavaCheck'.
> The following files had format violations:
src/test/org/apache/solr/core/BlobRepositoryMockingTest.java
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Fix test failure on J9: do not mock jre class [solr]
Posted by "mkhludnev (via GitHub)" <gi...@apache.org>.
mkhludnev commented on code in PR #2086:
URL: https://github.com/apache/solr/pull/2086#discussion_r1403754766
##########
solr/core/src/test/org/apache/solr/core/BlobRepositoryMockingTest.java:
##########
@@ -71,7 +71,8 @@ public void setUp() throws Exception {
super.setUp();
blobFetched = false;
blobKey = "";
- reset(mocks);
+ reset(mockContainer);
Review Comment:
we don't have `mocks` field anymore https://github.com/apache/solr/blame/main/solr/core/src/test/org/apache/solr/core/BlobRepositoryMockingTest.java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Fix test failure on J9: do not mock jre class [solr]
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on PR #2086:
URL: https://github.com/apache/solr/pull/2086#issuecomment-1823635256
I fixed the formatting, but the test changes in this commit still fails errorprone tests
```bash
$ gradle solr:core:check -x test -Pvalidation.errorprone=true
> Task :solr:core:compileTestJava
/Users/janhoy/git/solr/solr/core/src/test/org/apache/solr/core/BlobRepositoryMockingTest.java:117: warning: [DirectInvocationOnMock] Methods should not be directly invoked on the mock `mapMock`. Should this be part of a verify(..) call?
assertTrue(mapMock.get("foo!") instanceof BlobRepository.BlobContent);
^
(see https://errorprone.info/bugpattern/DirectInvocationOnMock)
/Users/janhoy/git/solr/solr/core/src/test/org/apache/solr/core/BlobRepositoryMockingTest.java:150: warning: [DirectInvocationOnMock] Methods should not be directly invoked on the mock `mapMock`. Should this be part of a verify(..) call?
mapMock.put("foo!", new BlobRepository.BlobContent<BlobRepository>("foo!", blobData));
[...]
```
Perhaps you can revert and then fix in a new PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Fix test failure on J9: do not mock jre class [solr]
Posted by "mkhludnev (via GitHub)" <gi...@apache.org>.
mkhludnev commented on PR #2086:
URL: https://github.com/apache/solr/pull/2086#issuecomment-1824921836
pushed fix into main and branch_9x
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Fix test failure on J9: do not mock jre class [solr]
Posted by "jdyer1 (via GitHub)" <gi...@apache.org>.
jdyer1 closed pull request #2086: Fix test failure on J9: do not mock jre class
URL: https://github.com/apache/solr/pull/2086
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Fix test failure on J9: do not mock jre class [solr]
Posted by "jdyer1 (via GitHub)" <gi...@apache.org>.
jdyer1 commented on PR #2086:
URL: https://github.com/apache/solr/pull/2086#issuecomment-1823074811
Thank you @madrob for the review. I cleaned a few things up, then pushed the commits via cherry-pick to "main" and "branch_9x".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Fix test failure on J9: do not mock jre class [solr]
Posted by "madrob (via GitHub)" <gi...@apache.org>.
madrob commented on code in PR #2086:
URL: https://github.com/apache/solr/pull/2086#discussion_r1401158281
##########
solr/core/src/test/org/apache/solr/core/BlobRepositoryMockingTest.java:
##########
@@ -151,15 +151,14 @@ public void testGetBlobIncrRefByUrl() throws Exception {
@Test
public void testCachedAlready() {
when(mockContainer.isZooKeeperAware()).thenReturn(true);
- when(mapMock.get("foo!"))
- .thenReturn(new BlobRepository.BlobContent<BlobRepository>("foo!", blobData));
+ mapMock.put("foo!", new BlobRepository.BlobContent<BlobRepository>("foo!", blobData));
BlobRepository.BlobContentRef<ByteBuffer> ref = repository.getBlobIncRef("foo!");
assertEquals("", blobKey);
assertFalse(blobFetched);
assertNotNull(ref.blob);
assertEquals(blobData, ref.blob.get());
verify(mockContainer).isZooKeeperAware();
- verify(mapMock).get("foo!");
+ assertTrue(mapMock.get("foo!") instanceof BlobRepository.BlobContent);
Review Comment:
nit: could be assertThat(mapMock.get, isInstance()) for better error message
##########
solr/core/src/test/org/apache/solr/core/BlobRepositoryMockingTest.java:
##########
@@ -71,7 +71,8 @@ public void setUp() throws Exception {
super.setUp();
blobFetched = false;
blobKey = "";
- reset(mocks);
+ reset(mockContainer);
Review Comment:
Can we clean up the assignments at line 51 and 54 since some of those are obsolete now I think?
##########
solr/core/src/test/org/apache/solr/core/BlobRepositoryMockingTest.java:
##########
@@ -191,8 +190,6 @@ public String getName() {
assertTrue(blobFetched);
assertNotNull(ref.blob);
assertEquals(PARSED, ref.blob.get());
- verify(mockContainer).isZooKeeperAware();
- verify(mapMock).get("foo!mocked");
- verify(mapMock).put(eq("foo!mocked"), any(BlobRepository.BlobContent.class));
+ verify(mockContainer).isZooKeeperAware();assertTrue(mapMock.get("foo!mocked") instanceof BlobRepository.BlobContent);
Review Comment:
nit: two statements on this line
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org