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.

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

ci(docs-infra): run tests against local Angular packages too #26202

Closed

Conversation

gkalpak
Copy link
Member

@gkalpak gkalpak commented Oct 1, 2018

This allows catching regressions earlier.

@gkalpak gkalpak added state: WIP area: build & ci Related the build and CI infrastructure of the project comp: docs-infra target: patch This PR is targeted for the next patch release labels Oct 1, 2018
@gkalpak gkalpak force-pushed the ci-aio-test-with-local-packages branch from 1dd3f31 to 4353aa7 Compare October 1, 2018 22:26
@mary-poppins
Copy link

You can preview 1dd3f31 at https://pr26202-1dd3f31.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 4353aa7 at https://pr26202-4353aa7.ngbuilds.io/.

@gkalpak gkalpak force-pushed the ci-aio-test-with-local-packages branch from 4353aa7 to a6e89a1 Compare October 2, 2018 08:38
@gkalpak gkalpak added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Oct 2, 2018
@gkalpak gkalpak changed the title WIP - ci(docs-infra): run tests against local Angular packages too ci(docs-infra): run tests against local Angular packages too Oct 2, 2018
@mary-poppins
Copy link

You can preview a6e89a1 at https://pr26202-a6e89a1.ngbuilds.io/.

@gkalpak gkalpak force-pushed the ci-aio-test-with-local-packages branch from a6e89a1 to db28ec1 Compare October 2, 2018 09:42
@mary-poppins
Copy link

You can preview db28ec1 at https://pr26202-db28ec1.ngbuilds.io/.

@gkalpak gkalpak force-pushed the ci-aio-test-with-local-packages branch from db28ec1 to 0a79555 Compare October 2, 2018 10:09
@mary-poppins
Copy link

You can preview 0a79555 at https://pr26202-0a79555.ngbuilds.io/.

@gkalpak gkalpak force-pushed the ci-aio-test-with-local-packages branch from 0a79555 to e3a9c05 Compare October 2, 2018 10:57
@mary-poppins
Copy link

You can preview e3a9c05 at https://pr26202-e3a9c05.ngbuilds.io/.

@@ -44,7 +44,7 @@ describe('ContributorService', () => {
it('contributors observable should complete', () => {
let completed = false;
contribService.contributors.subscribe(undefined, undefined, () => completed = true);
expect(true).toBe(true, 'observable completed');
expect(completed).toBe(true, 'observable completed');
Copy link
Member

Choose a reason for hiding this comment

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

❤️

${CI_MODE} == "aio_e2e" ||
${CI_MODE} == "aio_tools_test"
]]; then
# angular.io: Install all yarn dependencies according to angular.io/yarn.lock
travisFoldStart "yarn-install.aio"
(
# HACK (don't submit with this): Build Angular
./build.sh --packages=compiler,core,elements --examples=false

Copy link
Member

Choose a reason for hiding this comment

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

LOL

Copy link
Contributor

Choose a reason for hiding this comment

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

omg! :)

@@ -34,7 +35,7 @@ export class CopierService {
this.fakeElem.style[ isRTL ? 'right' : 'left' ] = '-9999px';

// Move element to the same position vertically
const yPosition = window.pageYOffset || document.documentElement.scrollTop;
const yPosition = window.pageYOffset || docElem.scrollTop;
Copy link
Member

Choose a reason for hiding this comment

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

I get the feeling that this change should be in a different commit ... 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 Agreed 😁
For the record, this was failing because of the updated lib.dom.d.ts typings in latest TS (which is brought in with aio-use-local).

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved it to a new commit, then it felt appropriate to upgrade TS to 3.1.x (which this change is related to), which in turn required upgrading Angular to 7.0.0-rc.0 😁

Now you have to review again 😛

Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

LGTM - but I wonder if we should be adding new jobs to CircleCI rather than Travis?
Or are we just not in a position to build AIO via CircleCI yet?

@IgorMinar IgorMinar added the action: merge The PR is ready for merge by the caretaker label Oct 5, 2018
@mary-poppins
Copy link

You can preview c18a77c at https://pr26202-c18a77c.ngbuilds.io/.

@petebacondarwin petebacondarwin 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 Oct 5, 2018
@jasonaden
Copy link
Contributor

@gkalpak Can you create a separate PR for patch? Specifically because of the changes to package.json and yarn.lock as these will never merge cleanly.

This was accidentally merged with 4d506ac and 87f60bc.
The build script is called in `scripts/ci/build.sh` (if necessary).
This is necessary to avoid webpack/webpack#8082, when installing
dependencies without taking the lockfile into account (e.g. with
`yarn aio-use-local` - locally or on CI).
@gkalpak gkalpak force-pushed the ci-aio-test-with-local-packages branch from c18a77c to 7b34786 Compare October 8, 2018 19:55
@gkalpak gkalpak added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release labels Oct 8, 2018
@mary-poppins
Copy link

You can preview 7b34786 at https://pr26202-7b34786.ngbuilds.io/.

@gkalpak
Copy link
Member Author

gkalpak commented Oct 8, 2018

Rebased this on master and changed PR target to master-only.
Created #26306 for patch.

@jasonaden jasonaden closed this in b46fa92 Oct 8, 2018
jasonaden pushed a commit that referenced this pull request Oct 8, 2018
jasonaden pushed a commit that referenced this pull request Oct 8, 2018
This is necessary to avoid webpack/webpack#8082, when installing
dependencies without taking the lockfile into account (e.g. with
`yarn aio-use-local` - locally or on CI).

PR Close #26202
@gkalpak gkalpak deleted the ci-aio-test-with-local-packages branch October 8, 2018 20:53
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
This was accidentally merged with 4d506ac and 87f60bc.
The build script is called in `scripts/ci/build.sh` (if necessary).

PR Close angular#26202
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
This is necessary to avoid webpack/webpack#8082, when installing
dependencies without taking the lockfile into account (e.g. with
`yarn aio-use-local` - locally or on CI).

PR Close angular#26202
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
@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 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: build & ci Related the build and CI infrastructure of the project cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants