Skip to content
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.

Sign up for GitHub

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

Jump to bottom

test(upgrade): log more info to help debug CI flakes #28045

Closed
wants to merge 1 commit into from
Closed

test(upgrade): log more info to help debug CI flakes #28045

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/upgrade/src/static/angular1_providers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import * as angular from '../common/angular1';
// We store the ng1 injector so that the provider in the module injector can access it
// Then we "get" the ng1 injector from the module injector, which triggers the provider to read
// the stored injector and release the reference to it.
let tempInjectorRef: angular.IInjectorService|null;
let tempInjectorRef: angular.IInjectorService|null = null;
export function setTempInjectorRef(injector: angular.IInjectorService) {
tempInjectorRef = injector;
}
Expand All @@ -21,7 +21,7 @@ export function injectorFactory() {
throw new Error('Trying to get the AngularJS injector before it being set.');
}

const injector: angular.IInjectorService|null = tempInjectorRef;
const injector: angular.IInjectorService = tempInjectorRef;
tempInjectorRef = null; // clear the value to prevent memory leaks
return injector;
}
Expand Down
21 changes: 16 additions & 5 deletions packages/upgrade/test/static/angular1_providers_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {Ng1Token} from '@angular/upgrade/static/src/common/angular1';
import {IInjectorService, Ng1Token} from '@angular/upgrade/static/src/common/angular1';
import {compileFactory, injectorFactory, parseFactory, rootScopeFactory, setTempInjectorRef} from '@angular/upgrade/static/src/static/angular1_providers';

{
Expand All @@ -22,19 +22,30 @@ import {compileFactory, injectorFactory, parseFactory, rootScopeFactory, setTemp

describe('injectorFactory', () => {
it('should return the injector value that was previously set', () => {
const mockInjector = {get: () => {}, has: () => false};
const mockInjector = {get: () => undefined, has: () => false};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what difference does this change make?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing 😁 (It's just considered good practice.)

Copy link
Contributor

@alfaproject alfaproject Jan 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as it doesn't get down leveled to ES5 (I know it doesn't in this case)

https://www.typescriptlang.org/play/index.html#src=const%20f%20%3D%20%7B%0A%20%20f1%3A%20()%20%3D%3E%20%7B%20%7D%2C%0A%20%20f2%3A%20()%20%3D%3E%20undefined%2C%20%2F%2F%20all%20your%20bytez%20are%20belong%20to%20us%0A%7D%3B%0A

setTempInjectorRef(mockInjector);
const injector = injectorFactory();
expect(injector).toBe(mockInjector);
});

it('should throw if the injector value has not been set yet', () => {
const mockInjector = {get: () => {}, has: () => false};
expect(injectorFactory).toThrowError();
let injector: IInjectorService|null = null;

try {
injector = injectorFactory();
} catch {
// Throwing an error is the expected behavior.
return;
}

// Normally, we should never get here (but sometimes we do on CI).
// Log some info to help debug the issue.
console.error(`Unexpected injector (${typeof injector}):`, injector);
fail(`Expected no injector, but got: ${jasmine.pp(injector)}`);
});

it('should unset the injector after the first call (to prevent memory leaks)', () => {
const mockInjector = {get: () => {}, has: () => false};
const mockInjector = {get: () => undefined, has: () => false};
setTempInjectorRef(mockInjector);
injectorFactory();
expect(injectorFactory).toThrowError(); // ...because it has been unset
Expand Down