-
Notifications
You must be signed in to change notification settings - Fork 237
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
Migrate to grunt build #498
Conversation
@KamilSzostak, |
@@ -58,7 +58,7 @@ module Microsoft.ApplicationInsights { | |||
* Adds telemetry initializer to the collection. Telemetry initializers will be called one by one | |||
* before telemetry item is pushed for sending and in the order they were added. |
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 think it would be useful to modify the comment to include the description of the function signature for telemetry initializer, specifically to indicate when it can return false/true or undefined.
this.setUserAgent("Googlebot/2.1"); | ||
} catch (ex) { | ||
Assert.ok(true, 'cannot run this test in the current setup - try Chrome'); | ||
return; |
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.
why do we need to catch and ok-return, instead of proceeding as if Beacon API is not supported?
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 need at least one Assert
otherwise, JQuery fails with an error - "No assertions executed...".
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 meant to catch this but proceed with remainder of the test as if Beacon API is not enabled
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.
This test validates if googlebot is detected correctly by the SDK. To do that I need to change the user agent which is not supported in phantomJs. There is nothing to validate if I cannot override the user agent.
The long term plan is to switch from phantomJS to headless Chrome, once it's available which supports user agent overriding.
@@ -0,0 +1,7 @@ | |||
page = require('webpage').create(); | |||
page.open('file:///E:/Dev/ApplicationInsights-JS/JavaScript/JavaScriptSDK.Tests/E2ETests/E2E.DisableTelemetryTests.htm?hidepassed', function () { |
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.
probably not intended to check this in?
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, I used this file to debug tests in PhantomJS.
@@ -1,13 +1,9 @@ | |||
| |||
Microsoft Visual Studio Solution File, Format Version 12.00 |
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.
Why do we still need SLN and *PROJ files?
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.
Never mind, I saw the comment that you put in saying this will be removed as part of subsequent changes.
.vscode/settings.json
Outdated
@@ -0,0 +1,8 @@ | |||
{ | |||
// Use Typescript from node_modules instead of builtin to VSCode |
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.
Comments aren't usually valid JSON. Is vscode okay with this?
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.
Removed.
@@ -1111,6 +1111,10 @@ class SenderTests extends TestClass { | |||
this.testCase({ | |||
name: "SenderTests: send() is using BeaconAPI sender if the BeaconAPI is enabled", | |||
test: () => { | |||
if (!(<any>navigator).sendBeacon) { |
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.
Are there types we can update rather than cast to any?
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 think I added that when I was fixing unit tests and forgot to remove it. It's gone now.
timing.responseStart = 30; | ||
timing.responseEnd = 42; | ||
timing.loadEventEnd = 60; | ||
(<any>timing).navigationStart = 1; |
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.
Why is this cast necessary?
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.
PerformanceTiming properties are read-only, to change them I need to cast them to any.
One way to fix this is to remove PerformanceTiming type and create a simple object with required properties. I like strongly typed objects, but in this case casting to any contradicts using a type, doesn't it?
var timing = {
navigationStart: 1,
connectEnd: 10,
requestStart: 11,
responseStart: 30,
responseEnd: 42,
loadEventEnd: 60,
};
instead of
var timing = <PerformanceTiming>{};
(<any>timing).navigationStart = 1;
(<any>timing).connectEnd = 10;
(<any>timing).requestStart = 11;
(<any>timing).responseStart = 30;
(<any>timing).responseEnd = 42;
(<any>timing).loadEventEnd = 60;
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 was this working before when it was strongly typed and not casted?
@@ -0,0 +1,11 @@ | |||
{ | |||
"compilerOptions": { |
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.
Any way to have grunt read from here rather than defining these options in multiple places?
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, I tried it at the beginning when I started the migration to grunt and I couldn't get it to work. But now when everything is building and test are passing it was much easier :)
Done:
grunt
to build ai.min and snippet scriptsgrunt module
to build the module scriptgrunt tests
to run unit and e2e testsTODO:
noImplicitAny: true