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

refactor(test): updated test helpers to be strongly typed #3265

Merged
merged 1 commit into from
Feb 14, 2018
Merged

refactor(test): updated test helpers to be strongly typed #3265

merged 1 commit into from
Feb 14, 2018

Conversation

david-driscoll
Copy link
Member

Description:
This allows tests to pick more on variation in type definitions when tests are being built

Related issue (if exists):

@rxjs-bot
Copy link

rxjs-bot commented Jan 26, 2018 •
edited
Loading

Warnings
⚠️

❗ Big PR (1)
Messages
📖

(1) : Pull Request size seems relatively large. If Pull Request contains multiple changes, split each into separate PR will helps faster, easier review.

CJS: 1358.3KB, global: 726.5KB (gzipped: 117.3KB), min: 140.3KB (gzipped: 30.6KB)

Generated by 🚫 dangerJS

@benlesh
Copy link
Member

benlesh commented Jan 26, 2018

@david-driscoll it looks like CI is freaking out on this one, but I like the spirit of it... can you have a look?

@kwonoj kwonoj mentioned this pull request Jan 26, 2018
Closed
@benlesh
Copy link
Member

benlesh commented Jan 30, 2018

Ping @david-driscoll

@kwonoj
Copy link
Member

kwonoj commented Jan 31, 2018

Check my proposal here too: #3266 - I'd rather moving forward to new test assertion and drop current one once migration's done.

@david-driscoll
Copy link
Member Author

@kwonoj I've updated and fixed the merging issues, however I'm getting this error in travis:

Merge commit detected.
Merge commit detected.
Request failed [404]: https://api.github.com/repos/ReactiveX/rxjs/statuses/369be8144cc597c26ee5808452a2178471fa7ead
Response: {
  "message": "Not Found",
  "documentation_url": "https://developer.github.com/v3/repos/statuses/#create-a-status"
}
Failing the build, there is 1 fail.

@coveralls
Copy link

coveralls commented Feb 13, 2018 •
edited
Loading

Coverage Status

Coverage remained the same at 97.502% when pulling 5ea026b on david-driscoll:typed-tests into c2b00f4 on ReactiveX:master.

@david-driscoll
Copy link
Member Author

I figured it out! 🎉

This should be good to go now.

@david-driscoll david-driscoll mentioned this pull request Feb 14, 2018
Merged
benlesh
benlesh requested changes Feb 14, 2018
View reviewed changes
package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "@reactivex/rxjs",
"version": "6.0.0-alpha.3",
"version": "6.0.0-alpha.2",
Copy link
Member

Choose a reason for hiding this comment

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

Oops. :D you probably didn't mean to do this.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah I think oj had something merged in, so I just took incoming changes. Fixed.

spec/observables/onErrorResumeNext-spec.ts Outdated
expectSubscriptions(source.subscriptions).toBe(subs);
});
});

Copy link
Member

Choose a reason for hiding this comment

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

Why is git freaking out about this file? It's going to ruin the blame.

Copy link
Member Author

Choose a reason for hiding this comment

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

Appears to be whitespace... the file as it exists today in master is using windows line breaks \r\n and when I change the file the .editorconfig forces the file to switch to just \n.

spec/observables/throwError-spec.ts Outdated
@@ -1,11 +1,10 @@
import { expect } from 'chai';
import * as Rx from '../../src/Rx';
import { TestScheduler } from '../../src/internal/testing/TestScheduler';
Copy link
Member

Choose a reason for hiding this comment

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

should be ../../src/testing

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed found 4 instances.

spec/operators/distinct-spec.ts
expectSubscriptions(e1.subscriptions).toBe(e1subs);
expectSubscriptions(e2.subscriptions).toBe(e2subs);
});
import * as Rx from '../../src/Rx';
Copy link
Member

Choose a reason for hiding this comment

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

Another one were git is freaking out... figure out what went wrong if you can.

Copy link
Member Author

Choose a reason for hiding this comment

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

CLRF vs LF

spec/operators/timestamp-spec.ts
expectObservable(e1.timestamp(rxTestScheduler)).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
});
import * as Rx from '../../src/Rx';
Copy link
Member

Choose a reason for hiding this comment

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

Another git freakout. lol

Copy link
Member Author

Choose a reason for hiding this comment

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

CLRF vs LF

@david-driscoll
refactor(test): updated test helpers to be strongly typed
5ea026b 
this allows tests to pick more on variation in type definitions when tests are being built
updated danger script to change how it finds cold and hot signatures
allow clrf files to be transparently converted to lf
@benlesh benlesh merged commit 4a86541 into ReactiveX:master Feb 14, 2018
@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@benlesh benlesh benlesh approved these changes

@kwonoj kwonoj Awaiting requested review from kwonoj

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

Successfully merging this pull request may close these issues.
5 participants
@david-driscoll @rxjs-bot @benlesh @kwonoj @coveralls