Fix as many skipped tests as possible. (#6524)

* chore: Stop skipping valid test

Resolves #5105

* chore: Add comment about why this test is skipped.

* chore: Correct skipped tests

- Some of the skipped tests were clearly copied from retryWhen but never updated appropriately.
- One of the skipped tests showed a buggy behavior that is now reported in an issue and a comment with a link to that issue is now above it.

* chore: Update skipped tests

- Uncomments a skipped test that was fixed with the v7 refactor (apparently just via better architecture)
- Adds an additional test to simplify the same assertion
- Adds a comment above a skipped section of tests explaining that we can probably remove them in v8 after we've moved completely to newer multicasting paradigms and removed deprecated operators.

Resolves #5370

* chore: Stops skipping a fixed test.

* chore: Add commented out bit to help find skipped tests

Figuring out what tests are skipped out of 3,000+ tests is pretty annoying. I want to leave this here as a sign post to help.

spec/Observable-spec.ts

@@ -880,29 +880,37 @@ describe('Observable', () => {
     });
   });
 
-  // TODO: Stop skipping this until a later refactor (probably in version 8)
   // Discussion here: https://github.com/ReactiveX/rxjs/issues/5370
-  it.skip('should emit an error for unhandled synchronous exceptions from something like a stack overflow', (done) => {
-    const source = of(4).pipe(
+  it('should handle sync errors within a test scheduler', () => {
+    const observable = of(4).pipe(
       map(n => {
-        if (n === 4) {
-          throw 'four!';
+          if (n === 4) {
+            throw 'four!';
         }
         return n;
       }),
-      catchError((_, source) => source),
+      catchError((err, source) => source),
     );
 
+    rxTestScheduler.run(helpers => {
+      const { expectObservable } = helpers;
+      expectObservable(observable).toBe('-');
+    });
+  });
+
+  it('should emit an error for unhandled synchronous exceptions from something like a stack overflow', () => {
+    const source = new Observable(() => {
+      const boom = (): unknown => boom();
+      boom();
+    });
+
     let thrownError: any = undefined;
-    try {
-      source.subscribe({
-        error: err => thrownError = err
-      });
-    } catch (err) {
-      done('Should never hit this!');
-    }
+    source.subscribe({
+      error: err => thrownError = err
+    });
 
-    expect(thrownError).to.equal(new RangeError('Maximum call stack size exceeded'));
+    expect(thrownError).to.be.an.instanceOf(RangeError);
+    expect(thrownError.message).to.equal('Maximum call stack size exceeded');
   });
 });
 
@@ -1191,6 +1199,9 @@ describe('Observable.lift', () => {
    * 
    * The problem is that you can have the Subject parts, or you can have the ConnectableObservable parts,
    * but you can't have both.
+   * 
+   * NOTE: We can remove this in version 8 or 9, because we're getting rid of operators that
+   * return `ConnectableObservable`. :tada:
    */
   describe.skip('The lift through Connectable gaff', () => {
     it('should compose through multicast and refCount, even if it is a Subject', () => {

spec/Subject-spec.ts

@@ -856,8 +856,7 @@ describe('useDeprecatedSynchronousErrorHandling', () => {
     }).to.throw(Error, 'lol');
   });
 
-  // TODO: This is still an issue. Not sure how to handle this one yet.
-  it.skip('should throw an error when notifying an complete, and concatenated with another observable that synchronously errors', () => {
+  it('should throw an error when notifying an complete, and concatenated with another observable that synchronously errors', () => {
     const subject = new Subject<string>();
     concat(subject, throwError(new Error('lol'))).subscribe();
 

spec/operators/repeatWhen-spec.ts

@@ -205,8 +205,8 @@ describe('repeatWhen operator', () => {
     expectObservable(result).toBe(expected);
   });
 
-  xit('should hide errors using a never notifier on a source with eventual error', () => {
-    const source = cold(  '--a--b--c--#');
+  it('should return a never-ending result if the notifier is never', () => {
+    const source = cold(  '--a--b--c--|');
     const subs =          '^          !';
     const notifier = cold(           '-');
     const expected =      '--a--b--c---------------------------------';
@@ -217,7 +217,7 @@ describe('repeatWhen operator', () => {
     expectSubscriptions(source.subscriptions).toBe(subs);
   });
 
-  xit('should propagate error thrown from notifierSelector function', () => {
+  it('should propagate error thrown from notifierSelector function', () => {
     const source = cold('--a--b--c--|');
     const subs =        '^          !';
     const expected =    '--a--b--c--#';
@@ -228,9 +228,8 @@ describe('repeatWhen operator', () => {
     expectSubscriptions(source.subscriptions).toBe(subs);
   });
 
-  xit('should replace error with complete using an empty notifier on a source ' +
-  'with eventual error', () => {
-    const source = cold(  '--a--b--c--#');
+  it('should complete if the notifier only completes', () => {
+    const source = cold(  '--a--b--c--|');
     const subs =          '^          !';
     const notifier = cold(           '|');
     const expected =      '--a--b--c--|';
@@ -277,7 +276,8 @@ describe('repeatWhen operator', () => {
     expectSubscriptions(source.subscriptions).toBe(subs);
   });
 
-  xit('should handle a hot source that raises error but eventually completes', () => {
+  // https://github.com/ReactiveX/rxjs/issues/6523
+  it.skip('should handle a host source that completes via operator like take, and a hot notifier', () => {
     const source =   hot('-1--2--3----4--5---|');
     const ssubs =       ['^      !            ',
                        '              ^    !'];
@@ -286,12 +286,7 @@ describe('repeatWhen operator', () => {
     const expected =     '-1--2---      -5---|';
 
     const result = source.pipe(
-      map((x: string) => {
-        if (x === '3') {
-          throw 'error';
-        }
-        return x;
-      }),
+      takeWhile(value => value !== '3'),
       repeatWhen(() => notifier)
     );
 

spec/operators/take-spec.ts

@@ -218,6 +218,8 @@ describe('take', () => {
     });
   });
 
+  // This is related to a PR with discussion here: https://github.com/ReactiveX/rxjs/pull/6396
+  // We can't fix this until version 8.
   it.skip('should unsubscribe from the source when it reaches the limit before a recursive synchronous upstream error is notified', () => {
     testScheduler.run(({ cold, expectObservable, expectSubscriptions }) => {
       const subject = new Subject();

spec/subjects/BehaviorSubject-spec.ts

@@ -193,7 +193,7 @@ describe('BehaviorSubject', () => {
     source.subscribe(subject);
   });
 
-  it.skip('should be an Observer which can be given to an interop source', (done) => {
+  it('should be an Observer which can be given to an interop source', (done) => {
     // This test reproduces a bug reported in this issue:
     // https://github.com/ReactiveX/rxjs/issues/5105
     // However, it cannot easily be fixed. See this comment:

spec/support/.mocharc.js

@@ -6,4 +6,6 @@ module.exports = {
   recursive: true,
   'enable-source-maps': true,
   'expose-gc': true,
+  // Uncomment this to find all skipped tests.
+  // forbidPending: true
 };