SCANCLI-216 Update scanner library to fix proxy authentication on CONNECT#267
SCANCLI-216 Update scanner library to fix proxy authentication on CONNECT#267
Conversation
hashicorp-vault-sonar-prod
bot
changed the title
henryju
force-pushed
the
jh/fix_proxy_connect_auth
branch
from
8e5cee6 to
a5d05ac
Compare
| @RunWith(Suite.class) | ||
| @SuiteClasses({ScannerTest.class, MultimoduleTest.class, | ||
| DistributionTest.class}) | ||
| DistributionTest.class, ProxyTest.class}) |
There was a problem hiding this comment.
Nitpick: I would prefer having one class per line
| private static final ConcurrentLinkedDeque<String> seenByProxy = new ConcurrentLinkedDeque<>(); | ||
| private static final ConcurrentLinkedDeque<String> seenConnectByProxy = new ConcurrentLinkedDeque<>(); |
There was a problem hiding this comment.
The names are unclear to me at first read. What are these queues?
There was a problem hiding this comment.
New attempt, let me know
| seenConnectByProxy.clear(); | ||
| } | ||
|
|
||
| private static void startProxy(boolean needProxyAuth) throws Exception { |
There was a problem hiding this comment.
Would be nice to not have to rely on a flag parameter here.
|
|
||
| private static Server proxyServer; | ||
| private static int httpProxyPort; | ||
| // HTTPS reverse-proxy target, used for the HTTPS CONNECT tests |
There was a problem hiding this comment.
I would avoid comments saying "what" as they should be replaced with a meaningful variable/method name.
| sslConnector.setPort(httpsTargetPort); | ||
| httpsTargetServer.addConnector(sslConnector); | ||
|
|
||
| // Transparently forward all requests to the Orchestrator instance |
There was a problem hiding this comment.
I would avoid comments saying "what" as they should be replaced with a meaningful variable/method name.
| httpsTargetServer.start(); | ||
| } | ||
|
|
||
| private static ServletContextHandler proxyHandler(boolean needProxyAuth) { |
There was a problem hiding this comment.
parameter flag should be avoided
There was a problem hiding this comment.
Did my best, not always possible IMO
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| public class ProxyTest extends ScannerTestCase { |
There was a problem hiding this comment.
The overall quality/readability of this test class is lacking IMO:
- The first Test occurrence happens at line 342 (red flag IMO)
- Unnecessary comments
- Flag params
- Vertical alignment of method calls is not respected
- Naming of methods is sometimes lacking the "start with a verb" principle, and is not entirely clear what the method is doing.
- Some methods are a bit too big
I can feel it was AI generated 😅
There was a problem hiding this comment.
In fact it should be a close copy of https://github.com/SonarSource/sonar-scanner-java-library/blob/master/its/it-tests/src/test/java/com/sonar/scanner/lib/it/ProxyTest.java
but I can still try to improve it ;)
There was a problem hiding this comment.
I made another attempt, let me know what you think.
|
2 participants
Please be aware that we are not actively looking for feature contributions. The truth is that it's extremely difficult for someone outside SonarSource to comply with our roadmap and expectations. Therefore, we typically only accept minor cosmetic changes and typo fixes. If you would like to see a new feature, please create a new thread in the forum "Suggest new features".
With that in mind, if you would like to submit a code contribution, make sure that you adhere to the following guidelines and all tests are passing:
We will try to give you feedback on your contribution as quickly as possible.
Thank You!
The SonarSource Team