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

Disable ng_package test temporarily #22933

Closed
wants to merge 2 commits into from
Closed

Disable ng_package test temporarily #22933

wants to merge 2 commits into from

Conversation

vicb
Copy link
Contributor

@vicb vicb commented Mar 22, 2018

Because it fails CircleCI with #22654

Also added a diff output to ease debugging.

(Part of) the error I observe locally is:

    --- esm2015/secondary/index.js ---
    @@ -429,25 +429,25 @@
      *
      * @suppress {suspiciousCode,uselessCode,missingProperties,missingOverride,checkTypes}
      * tslint:disable
      */ 
     import * as i0 from "@angular/core";
    -import * as i1 from "angular/packages/bazel/test/ng_package/example/secondary/secondarymodule";
    +import * as i1 from "example/secondary/secondarymodule";
     var SecondaryModuleNgFactory = i0.ɵcmf(i1.SecondaryModule, [], function (_l) { return i0.ɵmod([i0.ɵmpd(512, i0.ComponentFactoryResolver, i0.ɵCodegenComponentFactoryResolver, [[8, []], [3, i0.ComponentFactoryResolver], i0.NgModuleRef]), i0.ɵmpd(1073742336, i1.SecondaryModule, i1.SecondaryModule, [])]); });
     export { SecondaryModuleNgFactory as SecondaryModuleNgFactory };
     
    -//# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJmaWxlIjoic2Vjb25kYXJ5bW9kdWxlLm5nZmFjdG9yeS5qcyIsInNvdXJjZVJvb3QiOiIiLCJzb3VyY2VzIjpbIi4uLy4uLy4uLy4uLy4uLy4uLy4uLy4uLy4uL3BhY2thZ2VzL2JhemVsL3Rlc3QvbmdfcGFja2FnZS9leGFtcGxlL3NlY29uZGFyeS9zZWNvbmRhcnltb2R1bGUubmdmYWN0b3J5LnRzIl0sIm5hbWVzIjpbXSwibWFwcGluZ3MiOiIiLCJzb3VyY2VzQ29udGVudCI6WyJpbXBvcnQgKiBhcyBpMCBmcm9tICdAYW5ndWxhci9jb3JlJztcbmltcG9ydCAqIGFzIGkxIGZyb20gJ2FuZ3VsYXIvcGFja2FnZXMvYmF6ZWwvdGVzdC9uZ19wYWNrYWdlL2V4YW1wbGUvc2Vjb25kYXJ5L3NlY29uZGFyeW1vZHVsZSc7XG5leHBvcnQgY29uc3QgU2Vjb25kYXJ5TW9kdWxlTmdGYWN0b3J5OmkwLk5nTW9kdWxlRmFjdG9yeTxpMS5TZWNvbmRhcnlNb2R1bGU+ID0gKG51bGwgYXMgYW55KTtcbnZhciBfZGVjbDBfMDppMC5UZW1wbGF0ZVJlZjxhbnk+ID0gKDxhbnk+KG51bGwgYXMgYW55KSk7XG52YXIgX2RlY2wwXzE6aTAuRWxlbWVudFJlZjxhbnk+ID0gKDxhbnk+KG51bGwgYXMgYW55KSk7XG4iXX0=
    +//# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJmaWxlIjoic2Vjb25kYXJ5bW9kdWxlLm5nZmFjdG9yeS5qcyIsInNvdXJjZVJvb3QiOiIiLCJzb3VyY2VzIjpbIi4uLy4uLy4uLy4uLy4uLy4uLy4uLy4uLy4uL3BhY2thZ2VzL2JhemVsL3Rlc3QvbmdfcGFja2FnZS9leGFtcGxlL3NlY29uZGFyeS9zZWNvbmRhcnltb2R1bGUubmdmYWN0b3J5LnRzIl0sIm5hbWVzIjpbXSwibWFwcGluZ3MiOiIiLCJzb3VyY2VzQ29udGVudCI6WyJpbXBvcnQgKiBhcyBpMCBmcm9tICdAYW5ndWxhci9jb3JlJztcbmltcG9ydCAqIGFzIGkxIGZyb20gJ2V4YW1wbGUvc2Vjb25kYXJ5L3NlY29uZGFyeW1vZHVsZSc7XG5leHBvcnQgY29uc3QgU2Vjb25kYXJ5TW9kdWxlTmdGYWN0b3J5OmkwLk5nTW9kdWxlRmFjdG9yeTxpMS5TZWNvbmRhcnlNb2R1bGU+ID0gKG51bGwgYXMgYW55KTtcbnZhciBfZGVjbDBfMDppMC5UZW1wbGF0ZVJlZjxhbnk+ID0gKDxhbnk+KG51bGwgYXMgYW55KSk7XG52YXIgX2RlY2wwXzE6aTAuRWxlbWVudFJlZjxhbnk+ID0gKDxhbnk+KG51bGwgYXMgYW55KSk7XG4iXX0=

/ref #22814

@vicb vicb added action: review The PR is still awaiting reviews from at least one requested reviewer area: bazel Issues related to the published `@angular/bazel` build rules target: major This PR is targeted for the next major release labels Mar 22, 2018
alexeagle
alexeagle approved these changes Mar 22, 2018
View reviewed changes
packages/bazel/test/ng_package/example_package.spec.ts Outdated
Accept the new golden file:
bazel run ${process.env['BAZEL_TARGET']}.accept
`);

Diff:
Copy link
Contributor

Choose a reason for hiding this comment

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

would you move the diff first, so the easiest thing to see at the end of the output is how to accept the change?

packages/bazel/test/ng_package/example_package.spec.ts Outdated
@@ -49,11 +50,19 @@ if (require.main === module) {
if (actual === expected) {
return;
}

let patch = createPatch(goldenFile, expected, actual, 'Golden file', 'Generated file', {context: 5});
const endOfHeader = patch.indexOf('\n', patch.indexOf('\n') + 1) + 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

would be nice to comment why we do this - does the diff look bad otherwise (I know you're copy-pasting but don't want to blindly cargo-cult)

packages/bazel/test/ng_package/BUILD.bazel Outdated
@@ -48,6 +48,7 @@ jasmine_node_test(
"example_package.golden",
"//packages/bazel/test/ng_package/example:npm_package",
],
tags = ["manual"],
Copy link
Contributor

Choose a reason for hiding this comment

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

add a TODO here for me

@vicb vicb added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 22, 2018
@vicb vicb mentioned this pull request Mar 22, 2018
Closed
@mary-poppins
Copy link

You can preview ac822f8 at https://pr22933-ac822f8.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 6a5a178 at https://pr22933-6a5a178.ngbuilds.io/.

@vicb vicb added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Mar 22, 2018
@vicb
Copy link
Contributor Author

vicb commented Mar 22, 2018

merge-assistance: don't believe the bot. This PR is green.

@matsko matsko closed this in 4a075e8 Mar 22, 2018
matsko pushed a commit that referenced this pull request Mar 22, 2018
@vicb
test: temporarily disable ng_package test (#22933)
32105c8 
PR Close #22933
leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018
@vicb @leo6104
test: display a diff when example_apckage.spec.ts fails to ease debug…
94f94ac 
…ging (angular#22933)

PR Close angular#22933
leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018
@vicb @leo6104
test: temporarily disable ng_package test (angular#22933)
d637737 
PR Close angular#22933
@vicb vicb deleted the 0322-disTest branch April 3, 2018 17:59
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alexeagle alexeagle alexeagle approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: bazel Issues related to the published `@angular/bazel` build rules cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@vicb @mary-poppins @alexeagle @googlebot