Skip to content

Fix master testing and update eslint-config - #17367

Merged
skjnldsv merged 3 commits into
masterfrom
fix/master-eslint-tests
Oct 4, 2019
Merged

Fix master testing and update eslint-config#17367
skjnldsv merged 3 commits into
masterfrom
fix/master-eslint-tests

Conversation

@skjnldsv

Copy link
Copy Markdown
Member

After #17263

@ChristophWurstChristophWurst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🙈

@ChristophWurstChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 2, 2019
@skjnldsv

Copy link
Copy Markdown
MemberAuthor

I never seen a pr so fast approved! 😱 🚀

@skjnldsv

Copy link
Copy Markdown
MemberAuthor

/compile amend /

@skjnldsv

Copy link
Copy Markdown
MemberAuthor

Weird

Changes not staged for commit:
4728 | (use "git add <file>..." to update what will be committed)
4729 | (use "git checkout -- <file>..." to discard changes in working directory)
4730 | 4731 | modified: apps/workflowengine/js/workflowengine.js.map
4732

@skjnldsv
skjnldsvforce-pushed the fix/master-eslint-tests branch from 91e7a3a to 3a00b3aCompareOctober 2, 2019 09:42
@skjnldsv

Copy link
Copy Markdown
MemberAuthor

/compile amend /apps/workflowengine/js/

@skjnldsv
skjnldsvforce-pushed the fix/master-eslint-tests branch from 515e9ef to e90029bCompareOctober 2, 2019 10:38
@skjnldsv

Copy link
Copy Markdown
MemberAuthor

Ah, now I get it :)
We should split the tests between two: compiling and git diff

@skjnldsv
skjnldsvforce-pushed the fix/master-eslint-tests branch from 3849582 to 4376715CompareOctober 2, 2019 11:24
@skjnldsv

Copy link
Copy Markdown
MemberAuthor

/compile amend /

@skjnldsv

Copy link
Copy Markdown
MemberAuthor

@nextcloud/javascript I need help here
https://drone.nextcloud.com/nextcloud/server/22087/2/3

I don't understand!

modified: apps/workflowengine/js/workflowengine.js.map

@ChristophWurst

Copy link
Copy Markdown
Member

hold my php

@ChristophWurst

Copy link
Copy Markdown
Member

should be fine now

@skjnldsv

Copy link
Copy Markdown
MemberAuthor

should be fine now

any explanations?

@ChristophWurst

Copy link
Copy Markdown
Member

any explanations?

I ran

npm i

and then

npm run build

and the file in question was updated. So I amended.

@skjnldsv

Copy link
Copy Markdown
MemberAuthor

and the file in question was updated. So I amended.

yes, I did that 10 times!
Every time the file change. Also, I used the compile bot just before, so it should have done the exact same thing than you did :)

@ChristophWurst

Copy link
Copy Markdown
Member

Then let's see who's version wins 🍿

@skjnldsv

Copy link
Copy Markdown
MemberAuthor

Then let's see who's version wins

Ahahahaha
https://drone.nextcloud.com/nextcloud/server/22089/2/3

told you 😉

@skjnldsv
skjnldsvforce-pushed the fix/master-eslint-tests branch 3 times, most recently from 78a9698 to 24e47d4CompareOctober 3, 2019 08:36
@skjnldsv

Copy link
Copy Markdown
MemberAuthor

Capture d’écran_2019-10-03_10-44-38

🚀 🚀 🚀 🚀 🚀 🚀 🚀 🚀 🚀

*
*/
use OCA\WorkflowEngine\AppInfo\Application;
style(Application::APP_ID, 'multiselect');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So... I'm happy it works now. But it still feels hacky. Including the CSS into the bundle is much nicer IMO (as it saves requests). But lets debug that later.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Let's ping @juliushaertl 🙈

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Added it to the list to fix in #12790

@skjnldsv

skjnldsv commented Oct 3, 2019

Copy link
Copy Markdown
MemberAuthor

@danxuliu there are some acceptance tests failures.
No idea if they were here before or not. Could you check please?

--- Failed scenarios:
/nextcloud/tests/acceptance/features/app-comments.feature:171
/nextcloud/tests/acceptance/features/app-comments.feature:195
/nextcloud/tests/acceptance/features/app-files-sharing-link.feature:101
/nextcloud/tests/acceptance/features/app-files-sharing.feature:131
/nextcloud/tests/acceptance/features/app-files-sharing.feature:150
/nextcloud/tests/acceptance/features/app-files-sharing.feature:170
/nextcloud/tests/acceptance/features/app-files-sharing.feature:196
/nextcloud/tests/acceptance/features/app-files-sharing.feature:245
/nextcloud/tests/acceptance/features/app-files-tags.feature:31
/nextcloud/tests/acceptance/features/app-files.feature:22
/nextcloud/tests/acceptance/features/app-files.feature:108

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: npmbuildbot[bot] <npmbuildbot[bot]@users.noreply.github.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: npmbuildbot[bot] <npmbuildbot[bot]@users.noreply.github.com>
@skjnldsv
skjnldsvforce-pushed the fix/master-eslint-tests branch from 24e47d4 to 972279dCompareOctober 4, 2019 05:04
@skjnldsv

Copy link
Copy Markdown
MemberAuthor

@danxuliu there are some acceptance tests failures.
No idea if they were here before or not. Could you check please?

SHould be fixed now :)

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv

skjnldsv commented Oct 4, 2019

Copy link
Copy Markdown
MemberAuthor

Also fixed the acceptance-app-files-tags ^^
It was failing since #16682 (2 months)

@skjnldsv
skjnldsv merged commit 6d819e2 into masterOct 4, 2019
@skjnldsv
skjnldsv deleted the fix/master-eslint-tests branch October 4, 2019 06:18
@ChristophWurst

Copy link
Copy Markdown
Member

I would appreciate if we could have a second person review & approve the changes that happen after the reviews :)

@skjnldsv

Copy link
Copy Markdown
MemberAuthor

@ChristophWurst but there is 2 reviews 🤔

@blizzzblizzz mentioned this pull request Oct 4, 2019
16 tasks
@danxuliu

Copy link
Copy Markdown
Member

@danxuliu there are some acceptance tests failures.
No idea if they were here before or not. Could you check please?

They came from #17263. And I missed the mention and this pull request; I found the acceptance tests failures in another one. So after manually bisecting the commit (please, next time remove the ending semicolon in one commit, add the missing spaces in other, fix the documentation parameters in other... ;-) ) I finally found that the issue was caused by the destructuring assignment. And when I was happily going to open the pull request to fix it... then I saw that you already fixed it here! I want to cry :-P

Jokes aside, I would have expected Babel to transpile the destructuring assignment to something that older browsers could handle (as I guess that the same issue would happen in Internet Explorer 11). Maybe some rule or plugin is missing?

@skjnldsv

Copy link
Copy Markdown
MemberAuthor

I want to cry :-P

/me pat @danxuliu in the back 😁

Yeah, it's a bit weird! I reverted back the fix of the detructuring assignment and ignoroed eslint warning as this is a weird hack for old firefox entries. We shouldn't mess with this too much :)

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to releaseReady to be released and/or waiting for tests to finishbugfeature: dependencies

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@skjnldsv@ChristophWurst@danxuliu@rullzer@nickvergessen@juliusknorr