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

test: replace indexOf with includes and startsWith #13852

Closed
wants to merge 1 commit into from

Conversation

nataly87s
Copy link

changed all the tests that the issue #12586 was relevant to

this is my first contribution #goodnessSquad

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

@nodejs-github-bot nodejs-github-bot added addons Issues and PRs related to native addons. doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Jun 21, 2017
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Nice work. Please feel free to also give us any feedback about the onboarding process today.

All the changes LGTM

#goodnessSquad

@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt
Copy link
Contributor

CI seems OK, 2 unstable results due to a flaky test.

-1
);
assert.strictEqual(stderrOutput.indexOf(internalExMessage), -1);
assert(stderrOutput.includes(domainErrHandlerExMessage));
Copy link
Contributor

Choose a reason for hiding this comment

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

(non-blocking) Nit-Lite: Just for this case I'd use

assert.strictEqual(stderrOutput.includes(domainErrHandlerExMessage), true);

for the symmetry

@tniessen
Copy link
Member

tniessen commented Jul 2, 2017

@tniessen
Copy link
Member

tniessen commented Jul 2, 2017

@nataly87s It seems like you accidentally pushed a merge commit instead of your rebased changes. This makes it difficult to run CI against your changes and also requires a manual rebase while landing this PR. I assume you added https://github.com/nodejs/node.git as the upstream repository. If you did not, you can add it like this:

git remote add upstream https://github.com/nodejs/node.git

Please follow these steps to perform a proper rebase:

git reset --hard 44cfad0a5e41c99ab3bef41e5cf5b9c4f7ed742e
git fetch upstream
git rebase upstream/master

There will likely be some conflicts at this point. Running git status shows you a list of "Unmerged paths", the files with conflicts will be marked as "both modified". After fixing the rebase conflicts in these files, you can continue:

git rebase --continue

If everything worked, you should have a single commit on top of our upstream master containing your changes. You can push the changes to your repository using

git push origin master --force

By the way, you usually don't want to create pull requests from a master branch of your own repository. Most people create a new branch for each PR, this makes it simpler to work on multiple PRs at the same time and to rebase changes.

If you need further assistance, please let me know.

@tniessen tniessen self-assigned this Jul 2, 2017
@nataly87s
Copy link
Author

@tniessen Thanks, I'll do it as soon as I get to my laptop

@nataly87s
Copy link
Author

@tniessen I did the merge directly in github, but i tried to do what you said anyway...
When i tried to do

git remote add origin https://github.com/nodejs/node.git

I got this message

fatal: remote origin already exists.

I usually use SourceTree for anything related to git, so I'm kinda slow with it. Sorry ¯\(ツ)

@refack
Copy link
Contributor

refack commented Jul 2, 2017

git remote add origin https://github.com/nodejs/node.git

you actually need to do:

git remote add upstream https://github.com/nodejs/node.git

then do

git remove -v

should look something like:

d:\code\node-cur$ git remote -v
origin  https://github.com/refack/node.git (fetch)
origin  https://github.com/refack/node.git (push)
upstream        https://github.com/nodejs/node.git (fetch)
upstream        https://github.com/nodejs/node.git (push)

but with your handle

@tniessen
Copy link
Member

tniessen commented Jul 2, 2017

Sorry, my bad, @refack is correct, origin is your own repo. I also forgot that you need to force-push to master (instead of a normal push which won't let you overwrite existing history), I edited my comment.

@nataly87s
Copy link
Author

Ok, I did the rebase, but then

nataly-mac:node nataly$ git push origin master
To https://github.com/nataly87s/node.git
 ! [rejected]              master -> master (non-fast-forward)
error: failed to push some refs to 'https://github.com/nataly87s/node.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

@tniessen
Copy link
Member

tniessen commented Jul 2, 2017

@nataly87s That's what I am talking about ("force push"), you need to use git push --force origin master. This allows to overwrite existing history.

@nataly87s
Copy link
Author

Done.

@refack
Copy link
Contributor

refack commented Jul 2, 2017

So git protects you from trying to push something that is not a direct extension of the current tree (something with just more commit). There are two ways to override this:

  1. git push --force-with-lease will push it git can recognize all the commits you have locally and remotely (which is what happens when you rebase)
  2. The stronger git push --force or git push -f which will push you branch anyway.

I see by now @tniessen answer, but I'm posting anyway for context and posterity...

@refack
Copy link
Contributor

refack commented Jul 2, 2017

Done.

👍
image

@refack
Copy link
Contributor

refack commented Jul 2, 2017

How's the weather there? It's been quite Israeli here (Boston) this week (33° with 90% humidity), but with crazy 100mm 2-hour rain storms every other day, and golf ball sized hail ⛳️ once a week.

@refack
Copy link
Contributor

refack commented Jul 2, 2017

Setting it up for @tniessen to land: https://ci.nodejs.org/job/node-test-commit/10888/ 🏐

@nataly87s
Copy link
Author

I don't know whats the temperature, but it's hot as hell.
Even the water at the beach is next to boiling hot. But at least there's AC everywhere (except the beach)

This was referenced Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addons Issues and PRs related to native addons. doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants