You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2021/02/08 16:53:43 UTC
[GitHub] [cloudstack] GutoVeronezi opened a new pull request #4668: Adjust tests to allow builders containers
GutoVeronezi opened a new pull request #4668:
URL: https://github.com/apache/cloudstack/pull/4668
### Description
These tests were changing the value of an attribute in a protected static object at runtime instead of mocking the object. We discovered this problem when working with the docker build containers in https://github.com/khos2ow/cloudstack-deb-builder/pull/20. They were breaking the tests when building in the docker container.
This PR intends to improve these tests to use actual mocks, instead of change the value at runtime with reflection, keeping the same behavior.
Obs: The [DEB Builder](https://github.com/khos2ow/cloudstack-deb-builder) uses version 4.13 to test the container's building.
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [x] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [x] Minor
- [ ] Trivial
### How Has This Been Tested?
The tests were been ran and the packages were built locally.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GutoVeronezi commented on pull request #4668: Adjust tests to fix a problem with the container builders (https://github.com/khos2ow/cloudstack-deb-builder)
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on pull request #4668:
URL: https://github.com/apache/cloudstack/pull/4668#issuecomment-776719045
@DaanHoogland
Travis' tests are failing due to logs volume that extrapolate Travis' limit.
As this PR just improves a few test cases, I see no problem in merge it. What do you think?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GutoVeronezi commented on pull request #4668: Adjust tests to fix a problem with the container builders (https://github.com/khos2ow/cloudstack-deb-builder)
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on pull request #4668:
URL: https://github.com/apache/cloudstack/pull/4668#issuecomment-776642087
> code looks ok, do you intent for this to be merged forward as well, @GutoVeronezi ? and did you test it with newer versions as well?
@DaanHoogland
Yes, the intention is to do a fast forward of these changes to 4.14, 4.15 and master.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache edited a comment on pull request #4668: Adjust tests to fix a problem with the container builders (https://github.com/khos2ow/cloudstack-deb-builder)
Posted by GitBox <gi...@apache.org>.
weizhouapache edited a comment on pull request #4668:
URL: https://github.com/apache/cloudstack/pull/4668#issuecomment-785378394
> @DaanHoogland It is needed in 4.13 to fix [DEB Builder](https://github.com/khos2ow/cloudstack-deb-builder); however, it must be merged forward on 4.14, 4.15 and master too.
@GutoVeronezi I've pull docker image ubuntu1604 and ubuntu1804 from the repo you mentioned.
it seems it does not support cloudstack 4.14/4.15 and master, because it has openjdk8, but we use openjdk11 to build 4.14/4.15 and master.
my suggestion:
can you add "-DskipTests" to mvn command in the build-deb.sh ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GutoVeronezi commented on a change in pull request #4668: Adjust tests to fix a problem with the container builders (https://github.com/khos2ow/cloudstack-deb-builder)
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on a change in pull request #4668:
URL: https://github.com/apache/cloudstack/pull/4668#discussion_r575304316
##########
File path: plugins/ca/root-ca/src/test/java/org/apache/cloudstack/ca/provider/RootCAProviderTest.java
##########
@@ -133,17 +128,17 @@ public void testRevokeCertificate() throws Exception {
@Test
public void testCreateSSLEngineWithoutAuthStrictness() throws Exception {
- overrideDefaultConfigValue(RootCAProvider.rootCAAuthStrictness, "_defaultValue", "false");
+ provider.rootCAAuthStrictness = Mockito.mock(ConfigKey.class);
+ Mockito.when(provider.rootCAAuthStrictness.value()).thenReturn(Boolean.FALSE);
final SSLEngine e = provider.createSSLEngine(SSLUtils.getSSLContext(), "/1.2.3.4:5678", null);
- Assert.assertFalse(e.getUseClientMode());
Review comment:
@rhtyd actually if you take a look at this file in [4.15](https://github.com/apache/cloudstack/blob/4.15/plugins/ca/root-ca/src/test/java/org/apache/cloudstack/ca/provider/RootCAProviderTest.java), you will notice that this line is already removed, I made the changes using it as a reference.
My PR does not change the test case, it just change the way that variable is mocked.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4668: Adjust tests to fix a problem with the container builders (https://github.com/khos2ow/cloudstack-deb-builder)
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4668:
URL: https://github.com/apache/cloudstack/pull/4668#issuecomment-776533750
code looks ok, do you intent for this to be merged forward as well, @GutoVeronezi ? and did you test it with newer versions as well?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GutoVeronezi commented on pull request #4668: Adjust tests to fix a problem with the container builders (https://github.com/khos2ow/cloudstack-deb-builder)
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on pull request #4668:
URL: https://github.com/apache/cloudstack/pull/4668#issuecomment-785063110
@DaanHoogland @rhtyd can we get a feedback here? This is a simple PR, yet vital to fix [DEB Builder](https://github.com/khos2ow/cloudstack-deb-builder).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4668: Adjust tests to fix a problem with the container builders (https://github.com/khos2ow/cloudstack-deb-builder)
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4668:
URL: https://github.com/apache/cloudstack/pull/4668#issuecomment-777334821
> @DaanHoogland
> Travis' tests are failing due to logs volume that extrapolate Travis' limit.
> As this PR just improves a few test cases, I see no problem in merge it. What do you think?
I agree , but 4.14 is in freeze for release now, cc @rhtyd can you comment?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GutoVeronezi commented on pull request #4668: Adjust tests to fix a problem with the container builders (https://github.com/khos2ow/cloudstack-deb-builder)
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on pull request #4668:
URL: https://github.com/apache/cloudstack/pull/4668#issuecomment-786621768
All checks of https://github.com/khos2ow/cloudstack-deb-builder/pull/20 have passed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GutoVeronezi edited a comment on pull request #4668: Adjust tests to fix a problem with the container builders (https://github.com/khos2ow/cloudstack-deb-builder)
Posted by GitBox <gi...@apache.org>.
GutoVeronezi edited a comment on pull request #4668:
URL: https://github.com/apache/cloudstack/pull/4668#issuecomment-786621768
With this changes all checks of https://github.com/khos2ow/cloudstack-deb-builder/pull/20 have passed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #4668: Adjust tests to fix a problem with the container builders (https://github.com/khos2ow/cloudstack-deb-builder)
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #4668:
URL: https://github.com/apache/cloudstack/pull/4668#issuecomment-785423569
> > @DaanHoogland It is needed in 4.13 to fix [DEB Builder](https://github.com/khos2ow/cloudstack-deb-builder); however, it must be merged forward on 4.14, 4.15 and master too.
>
> @GutoVeronezi I've pull docker image ubuntu1604 and ubuntu1804 from the repo you mentioned.
> it seems it does not support cloudstack 4.14/4.15 and master, because it has openjdk8, but we use openjdk11 to build 4.14/4.15 and master.
>
> my suggestion:
> can you add "-DskipTests" to mvn command in the build-deb.sh ?
I tested the following, it works
(1) checkout 4.15
(2) changed mvn command in tools/docker/Dockerfile to `RUN mvn -Pdeveloper -Dsimulator clean install`
(3) run 'docker build -f tools/docker/Dockerfile -t cloudstack:4.15 .', all are good.
so I think 4.15 does not have this issue.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rafaelweingartner merged pull request #4668: Adjust tests to fix a problem with the container builders (https://github.com/khos2ow/cloudstack-deb-builder)
Posted by GitBox <gi...@apache.org>.
rafaelweingartner merged pull request #4668:
URL: https://github.com/apache/cloudstack/pull/4668
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GutoVeronezi commented on pull request #4668: Adjust tests to fix a problem with the container builders (https://github.com/khos2ow/cloudstack-deb-builder)
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on pull request #4668:
URL: https://github.com/apache/cloudstack/pull/4668#issuecomment-785348210
@DaanHoogland It is needed in 4.13 to fix [DEB Builder](https://github.com/khos2ow/cloudstack-deb-builder); however, it must be merged forward on 4.14, 4.15 and master too.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4668: Adjust tests to fix a problem with the container builders (https://github.com/khos2ow/cloudstack-deb-builder)
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4668:
URL: https://github.com/apache/cloudstack/pull/4668#issuecomment-785097561
@GutoVeronezi Do i understand correctly that this need not be merged forward? (if it works for you we can merge but 4.13 is out of maintenance)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #4668: Adjust tests to fix a problem with the container builders (https://github.com/khos2ow/cloudstack-deb-builder)
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #4668:
URL: https://github.com/apache/cloudstack/pull/4668#issuecomment-785378394
> @DaanHoogland It is needed in 4.13 to fix [DEB Builder](https://github.com/khos2ow/cloudstack-deb-builder); however, it must be merged forward on 4.14, 4.15 and master too.
@GutoVeronezi I've pushed docker image ubuntu1604 and ubuntu1804 from the repo you mentioned.
it seems it does not support cloudstack 4.14/4.15 and master, because it has openjdk8, but we use openjdk11 to build 4.14/4.15 and master.
my suggestion:
can you add "-DskipTests" to mvn command in the build-deb.sh ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #4668: Adjust tests to fix a problem with the container builders (https://github.com/khos2ow/cloudstack-deb-builder)
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #4668:
URL: https://github.com/apache/cloudstack/pull/4668#issuecomment-785764975
the test issue might have been fixed in 4.14+ by #4226
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on a change in pull request #4668: Adjust tests to fix a problem with the container builders (https://github.com/khos2ow/cloudstack-deb-builder)
Posted by GitBox <gi...@apache.org>.
rhtyd commented on a change in pull request #4668:
URL: https://github.com/apache/cloudstack/pull/4668#discussion_r575032411
##########
File path: plugins/ca/root-ca/src/test/java/org/apache/cloudstack/ca/provider/RootCAProviderTest.java
##########
@@ -133,17 +128,17 @@ public void testRevokeCertificate() throws Exception {
@Test
public void testCreateSSLEngineWithoutAuthStrictness() throws Exception {
- overrideDefaultConfigValue(RootCAProvider.rootCAAuthStrictness, "_defaultValue", "false");
+ provider.rootCAAuthStrictness = Mockito.mock(ConfigKey.class);
+ Mockito.when(provider.rootCAAuthStrictness.value()).thenReturn(Boolean.FALSE);
final SSLEngine e = provider.createSSLEngine(SSLUtils.getSSLContext(), "/1.2.3.4:5678", null);
- Assert.assertFalse(e.getUseClientMode());
Review comment:
That's essentially removing the test case, I would actually disable or remove the test @GutoVeronezi
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rafaelweingartner commented on pull request #4668: Adjust tests to fix a problem with the container builders (https://github.com/khos2ow/cloudstack-deb-builder)
Posted by GitBox <gi...@apache.org>.
rafaelweingartner commented on pull request #4668:
URL: https://github.com/apache/cloudstack/pull/4668#issuecomment-785794401
@weizhouapache the problem we discovered was found with Java8. Anyways, the goal here is to patch a poorly coded test that is breaking Java8 docker build. Anybody that look at the code will see that it is not using mock properly. Instead, the unit test case is "mockig" via manual reflection. We need to patch this on 4.13, so we can fix the build of the docker build images with Java8; these images are validated with the latest Java8 ACS version. Then, @GutoVeronezi is introducing a new docker build configuration that uses Java11 there as well, which will be validated with Java11.
Why does the test only breaks on OpenJDK8? We have no idea, when I reviewed and saw the test cases, my OCD kicked in, ant that was the first thing fixed. When we tested again, it worked. Anyways, I will merge this one later today. It is a simple patch that is fixing a test case that was poorly written.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org