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(operator-concatAll): use run mode #5144

Open
wants to merge 3 commits into
base: master
from

Conversation

@ZackDeRose
Copy link

ZackDeRose commented Nov 15, 2019

Description:

Uses run mode for concatAll operator tests.

Related issue (if exists):

spec/operators/concatAll-spec.ts Outdated Show resolved Hide resolved
@ZackDeRose

This comment has been minimized.

Copy link
Author

ZackDeRose commented Nov 15, 2019

@cartant added fixes to whitespace - do y'all squash and merge? Or should i look to rebase to make this a single commit?

@cartant

This comment has been minimized.

Copy link
Collaborator

cartant commented Nov 15, 2019

We squash 'em, so use as many commits as you like. I'll review this later, when I'm on a desktop. Thanks, Zack.

Copy link
Collaborator

cartant left a comment

A few nits about whitespace. I noticed that some of the marble alignments seemed wrong in the existing code. However, I think we can let that slide. I'm happy for this PR to address the run and Prettier-related changes only. (Although, if you see something that looks wrong with the existing alignments, feel free to correct them to what you think they ought to be. Entropy has messed them up a little.)

y: cold(' c-d-e-f-#'),
z: cold(' g-h-i-j-k-|')
};
const e1 = hot('--x---------y--------z--------|', values);

This comment has been minimized.

Copy link
@cartant

cartant Nov 15, 2019

Collaborator

There are multiple spaces between the = and hot.

testScheduler.run(({ cold, expectObservable, expectSubscriptions }) => {
const e1 = cold('-');
const e1subs = '^';
const e2 = cold('--|');

This comment has been minimized.

Copy link
@cartant

cartant Nov 15, 2019

Collaborator

Whitespace.

testScheduler.run(({ cold, expectObservable, expectSubscriptions }) => {
const e1 = cold('-');
const e1subs = '^';
const e2 = cold('-');

This comment has been minimized.

Copy link
@cartant

cartant Nov 15, 2019

Collaborator

Whitespace.

testScheduler.run(({ cold, expectObservable, expectSubscriptions }) => {
const e1 = cold('-');
const e1subs = '^';
const e2 = cold('--a--|');

This comment has been minimized.

Copy link
@cartant

cartant Nov 15, 2019

Collaborator

Whitespace.

@@ -2,24 +2,33 @@ import { expect } from 'chai';
import { from, throwError, of, Observable } from 'rxjs';
import { concatAll, take, mergeMap } from 'rxjs/operators';
import { TestScheduler } from 'rxjs/testing';
import { hot, cold, expectObservable, expectSubscriptions } from '../helpers/marble-testing';
import { observableMatcher } from 'spec/helpers/observableMatcher';
import { expectObservable } from '../helpers/marble-testing';

This comment has been minimized.

Copy link
@cartant

cartant Nov 15, 2019

Collaborator

It's likely that the IDE added the ../helpers/marble-testing import automatically (and unnecessarily). We shouldn't need anything from this file once we're using run.

@ZackDeRose ZackDeRose force-pushed the ZackDeRose:concat-all-run-mode branch from 6cfe12a to 9c4e255 Nov 19, 2019
@ZackDeRose

This comment has been minimized.

Copy link
Author

ZackDeRose commented Nov 19, 2019

@cartant - Enabled prettier to help find/format away the whitespaces, it ended up reformatting in some other places as well.

@cartant

This comment has been minimized.

Copy link
Collaborator

cartant commented Nov 19, 2019

Yeah, I don't think we want to run Prettier just yet. It will make the reviews too hard. I would prefer to just make the run-mode changes and fix the out-of-literal white spaces.

@benlesh

This comment has been minimized.

Copy link
Member

benlesh commented Nov 21, 2019

That's funny, @cartant ... I was going to suggest just running Prettier on the one file. One of the only reasons we don't have Prettier (or the like) in our repo is because of all of the old marble tests and how they rely on whitespace outside of strings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.