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

T/209: Upgraded dependencies #246

Merged
merged 12 commits into from
Aug 9, 2017
Merged

T/209: Upgraded dependencies #246

merged 12 commits into from
Aug 9, 2017

Conversation

pomek
Copy link
Member

@pomek pomek commented Jul 31, 2017

Suggested merge commit message (convention)

Internal: Upgraded dependencies. Closes #209. Closes #236. Closes #237.

@pomek pomek requested a review from Reinmar July 31, 2017 07:20
@pomek
Copy link
Member Author

pomek commented Jul 31, 2017

R- because of some issues with CI occurred.

Kamil Piechaczek added 2 commits July 31, 2017 11:58
Fixed issues with mocks in tests.
b019de8 
Some mocks from one test affected to the another one.
Code style fixes.
f400a56
@pomek
Copy link
Member Author

pomek commented Jul 31, 2017

Ok, ready.

@Reinmar
Copy link
Member

Reinmar commented Aug 4, 2017

You missed a couple of packages:

image

@Reinmar
Copy link
Member

Reinmar commented Aug 4, 2017

I'll try to update these myself.

@Reinmar
Copy link
Member

Reinmar commented Aug 4, 2017

OK, I'm predicting a problem. Chain and Sinon are installed by ckeditor5-dev-tests. The problem is that they are used both for that package's tests and for the CKE5 tests. So when we'll update them they will crash CKE5's tests. Am I right?

@pomek
Copy link
Member Author

pomek commented Aug 4, 2017

I hope no because Sinon and Chain are defined in devDependencies. But I'm not sure…

@Reinmar
Copy link
Member

Reinmar commented Aug 4, 2017

Yes, I was right. After I updated these dependencies so ckeditor5-dev-tests uses the same version of Chai and Sinon in prod and dev modes the CKE5 tests start to throw.

Currently, both packages are listed twice in package.json which is bad because I'm often using dev versions of ckeditor5-dev-* packages in ckeditor5. We need these versions to be consistent.

So:

  1. I pushed a commit here updating the remaining dependencies and deduping them.
  2. You need to fix ckeditor5-dev-* tests because some of them log Sinon warnings.
  3. You need to actually test your changes in ckeditor5. I think that the easiest way to do that in a reliable way is to: 
    cd ckeditor5
# add "../ckeditor5-dev/packages" to lerna.json so Lerna symlinks packages installed in 
ckeditor5 to those available in this repo.
lerna bootstrap
  4. Test all gulp tasks which you can test (so all except the release task).

Only then this PR will make sense.

@pomek
Copy link
Member Author

pomek commented Aug 7, 2017

You need to fix ckeditor5-dev-* tests because some of them log Sinon warnings.

Done in eb50099.

Test all gulp tasks which you can test (so all except the release task).

  • lint
  • lint-staged
  • pre-commit
  • test
  • test:manual
  • docs
  • docs:api
  • translations:collect
  • translations:upload
  • translations:download
  • changelog:dependencies
  • release:dependencies

Because I created the release tool tasks, I know how and when I can break the process without any effects. But with the translations, I don't even want to call these tasks.

After calling npm t in the main repository, 10 tests fail and a lot of them log Sinon warnings. These warnings are easy to fix but the fail tests can be hard to fix.

ma2ciek
ma2ciek reviewed Aug 7, 2017
View reviewed changes
packages/ckeditor5-dev-utils/tests/workspace.js Outdated
@@ -60,7 +60,7 @@ describe( 'utils', () => {
it( 'should return only ckeditor5 directories', () => {
const workspacePath = '/workspace/path';
const sourceDirectories = [ 'tools', 'ckeditor5', 'ckeditor5-core', '.bin', 'ckeditor5-plugin-image' ];
sandbox.stub( tools, 'getDirectories', () => sourceDirectories );
sandbox.stub( tools, 'getDirectories' ).callsFake( () => sourceDirectories );
Copy link
Contributor

Choose a reason for hiding this comment

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

It can be just .returns( sourceDirectories ). Am I right?

@pomek
Copy link
Member Author

pomek commented Aug 9, 2017 •
edited
Loading

I fixed all the tests. After merging this PR, would be nice to read the migration guide from 1.x to 2.x and from 2.x to 3.x.

Along with the changes in this PR, the following branches in the repositories have to merged:

Unfortunately, upgrade Sinon to the latest version caused one error in our test. We cannot stub Array.prototype.filter (see) because of Maximum call stack size exceeded. I reported this bug – sinonjs/sinon#1521.

@Reinmar
Copy link
Member

Reinmar commented Aug 9, 2017

Unfortunately, upgrade Sinon to the latest version caused one error in our test. We cannot stub Array.prototype.filter (see) because of Maximum call stack size exceeded. I reported this bug – sinonjs/sinon#1521.

So does this test pass now or not? If not, then we need to look for workaround. We cannot leave it like this.

@pomek
Copy link
Member Author

pomek commented Aug 9, 2017

So does this test pass now or not? If not, then we need to look for workaround. We cannot leave it like this.

All works fine. I did a workaround.

@pomek
Copy link
Member Author

pomek commented Aug 9, 2017

image

@Reinmar
Copy link
Member

Reinmar commented Aug 9, 2017 •
edited
Loading

To be reviewed:

@pomek
Copy link
Member Author

pomek commented Aug 9, 2017

mgit.json

{
  "packages": "packages/",
  "dependencies": {
    "@ckeditor/ckeditor5-adapter-ckfinder": "ckeditor/ckeditor5-adapter-ckfinder#t/ckeditor5-dev/209",
    "@ckeditor/ckeditor5-autoformat": "ckeditor/ckeditor5-autoformat",
    "@ckeditor/ckeditor5-basic-styles": "ckeditor/ckeditor5-basic-styles",
    "@ckeditor/ckeditor5-block-quote": "ckeditor/ckeditor5-block-quote",
    "@ckeditor/ckeditor5-build-classic": "ckeditor/ckeditor5-build-classic",
    "@ckeditor/ckeditor5-clipboard": "ckeditor/ckeditor5-clipboard",
    "@ckeditor/ckeditor5-core": "ckeditor/ckeditor5-core#t/ckeditor5-dev/209",
    "@ckeditor/ckeditor5-editor-balloon-toolbar": "ckeditor/ckeditor5-editor-balloon-toolbar#t/ckeditor5-dev/209",
    "@ckeditor/ckeditor5-editor-classic": "ckeditor/ckeditor5-editor-classic",
    "@ckeditor/ckeditor5-editor-inline": "ckeditor/ckeditor5-editor-inline",
    "@ckeditor/ckeditor5-engine": "ckeditor/ckeditor5-engine#t/ckeditor5-dev/209",
    "@ckeditor/ckeditor5-enter": "ckeditor/ckeditor5-enter",
    "@ckeditor/ckeditor5-heading": "ckeditor/ckeditor5-heading",
    "@ckeditor/ckeditor5-image": "ckeditor/ckeditor5-image",
    "@ckeditor/ckeditor5-link": "ckeditor/ckeditor5-link#t/ckeditor5-dev/209",
    "@ckeditor/ckeditor5-list": "ckeditor/ckeditor5-list",
    "@ckeditor/ckeditor5-markdown-gfm": "ckeditor/ckeditor5-markdown-gfm",
    "@ckeditor/ckeditor5-paragraph": "ckeditor/ckeditor5-paragraph",
    "@ckeditor/ckeditor5-presets": "ckeditor/ckeditor5-presets",
    "@ckeditor/ckeditor5-theme-lark": "ckeditor/ckeditor5-theme-lark",
    "@ckeditor/ckeditor5-typing": "ckeditor/ckeditor5-typing",
    "@ckeditor/ckeditor5-ui": "ckeditor/ckeditor5-ui#t/ckeditor5-dev/209",
    "@ckeditor/ckeditor5-undo": "ckeditor/ckeditor5-undo",
    "@ckeditor/ckeditor5-upload": "ckeditor/ckeditor5-upload#t/ckeditor5-dev/209",
    "@ckeditor/ckeditor5-utils": "ckeditor/ckeditor5-utils#t/ckeditor5-dev/209",
    "@ckeditor/ckeditor5-widget": "ckeditor/ckeditor5-widget#t/ckeditor5-dev/209"
  }
}

@Reinmar
Copy link
Member

Reinmar commented Aug 9, 2017

Incorrect changes:

@pomek
Copy link
Member Author

pomek commented Aug 9, 2017

Fixed.

@Reinmar
Copy link
Member

Reinmar commented Aug 9, 2017

All green and looks fine.

@Reinmar Reinmar merged commit 44d06ba into master Aug 9, 2017
@Reinmar Reinmar deleted the t/209 branch August 9, 2017 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ma2ciek ma2ciek ma2ciek left review comments

@Reinmar Reinmar Awaiting requested review from Reinmar

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pomek @Reinmar @ma2ciek