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

Fix tests during edit - Ready for Review / Merge @muxator #3745

Merged
merged 14 commits into from
Mar 24, 2020

Conversation

JohnMcLear
Copy link
Member

@JohnMcLear JohnMcLear commented Mar 22, 2020

When the test framework requested bold.js it would serve the swap file before the bold.js file causing developers to get confused as to why their changes to spec files weren't being represented upon page refresh. To fix I remove swap files from the list of files used by Etherpad.

Also....

Tests: What's changed and why?

Now our Sauce Labs tests worked it seemed like a suitable time to fix tests which over the years have become broken due to browser changes.

Also you might want to squash these commits.

@JohnMcLear
Copy link
Member Author

Not sure why the sauce labs commit is in? :D Anyway, this is the fix for the issue experienced in #3730

@JohnMcLear JohnMcLear mentioned this pull request Mar 23, 2020
@JohnMcLear JohnMcLear changed the title Fix tests during edit Fix tests during edit - Ready for Review / Merge @muxator Mar 23, 2020
@muxator
Copy link
Contributor

muxator commented Mar 23, 2020

Not sure why the sauce labs commit is in? :D Anyway, this is the fix for the issue experienced in #3730

I came here to say it... by mistake this series was based on the saucelab enablement branch #3743.
I'll have to rebase it. It's a bit nasty to do this in the public repository, but this is a secondary branch after all.

If possible, I'd prefer PR based on private repos so they can be meddled with.

@JohnMcLear
Copy link
Member Author

Yea see comment on other PR, it's because I'm working on sauce labs which means I have to commit to ether/ to get the travis runner to init

@muxator muxator force-pushed the fix-tests-during-edit branch 2 times, most recently from 170ef9c to 1a26260 Compare March 23, 2020 21:47
@muxator
Copy link
Contributor

muxator commented Mar 23, 2020

After this one, #3743 and #3749 are merged it would be better if you start over from a new clone, because otherwise there will be dragons ahead. I'll ping you.

@JohnMcLear
Copy link
Member Author

Yea that's fine, let's get these in so we can keep moving on. Once these are done nearly all future commits will land from JohnMcLear/

@JohnMcLear
Copy link
Member Author

Can this get merged in or do you want a new clone? I don't see any conflicts but I'm mindful of # of commits. (afaik you can squash these on merge)

@muxator
Copy link
Contributor

muxator commented Mar 23, 2020

do you want a new clone?

Not for this PR. It's not worth the effort since I'll have to rewrite the branch and delete it anyway.

After everything it's merged, I advise you to re-clone the repo on your dev machine because otherwise you'll likey will see fun stuff happening. :)

@@ -136,7 +136,7 @@ describe("the test helper", function(){
helper.selectLines($startLine, $endLine, startOffset, endOffset);

var selection = inner$.document.getSelection();
expect(cleanText(selection.toString())).to.be("ort lines to t");
expect(cleanText(selection.toString().replace(/(\r\n|\n|\r)/gm,""))).to.be("ort lines to t");
Copy link
Contributor

Choose a reason for hiding this comment

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

This is logically part of the next commit where you do the same in other places, right?

I'll move it there and comment from that point.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear, I am referring to this: 6a7bcfd

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea.

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'm terrible at committing, I drank too much of the "commit often" juice.. 💯 Sorry about that! At least I show my work tho, I like to show how dumb I can be / how my brain goes about solving / sorting things so when I have to fix something (in this case 6 years later?) I can see what I was trying to do and why I did something weird.

I do a lot of weird. But that's okay 🍡

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem. I'm just trying to make sure we won't get lost in another 6 years... :)

muxator and others added 2 commits March 24, 2020 01:04
This is a followup on 312c72c, which did the same on the main code base,
and is preliminary work for tidying up John's changes in the following commits.

No functional changes.

Command:
    find tests/frontend -name '*.js' -type f -print0 | xargs -0 sed --in-place 's/[[:space:]]*$//'
This helps during plugin development, which otherwise which break when a
developer is editing a test.
@muxator
Copy link
Contributor

muxator commented Mar 24, 2020

Geez, that's a lot of work you have done! I'll continue tomorrow.

@JohnMcLear
Copy link
Member Author

@muxator thanks, lemme know if you get stuck, I have some more tests to merge / send PR for and I don't want to work against a volatile code base.

@muxator
Copy link
Contributor

muxator commented Mar 24, 2020

I am taking these, with just a minimum rewrite.

I still have some doubts, but I understand you are still working on it and I do not want to block your progress.

Pulled in, thanks.

@muxator muxator merged commit 3c78ada into develop Mar 24, 2020
@muxator muxator deleted the fix-tests-during-edit branch March 24, 2020 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants