-
Notifications
You must be signed in to change notification settings - Fork 205
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
fix quickpulse url construction. #1090
Conversation
add tests to verify.
@trask, I had to make quite a few changes to prevent static variable values from bleeding into other tests which were causing failures. Ping me if you want me to go through the details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like the tests! Just comments about Intellij formatting below.
import org.junit.BeforeClass; | ||
import org.junit.Test; | ||
|
||
import org.junit.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you change Intellij settings? I think default is 99 imports before it uses '*'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. For tests, I allow * for org.junit
, org.hamcrest
and org.mockito
and there are a few static ones too, e.g. org.junit.Assert.*
. For prod code, I prefer to be explicit. For tests I like the splats for the test framework classes. Intellisense is usually quicker/better with its suggestions.
I think the checkstyle settings reflect this, but maybe not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you configure this differently in Intellij for test vs prod?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you can. I have the count settings set to 500 (default is 99 but that didn't always work for me), plus rules for org.junit, Mockito, Matchers and Assert which always use *
. These settings are under Editor/Code Style/Java/Imports
import org.junit.Before; | ||
import org.junit.Test; | ||
|
||
import org.junit.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here re: Intellij settings
@@ -1,11 +1,15 @@ | |||
package com.microsoft.applicationinsights.internal.quickpulse; | |||
|
|||
import com.microsoft.applicationinsights.TelemetryConfiguration; | |||
import org.junit.Assert; | |||
import org.junit.Test; | |||
import org.junit.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here re: Intellij settings
add tests to verify.
Fix #1089