Skip to content

Conversation

@lw-lin
Copy link
Contributor

@lw-lin lw-lin commented Nov 13, 2016

What changes were proposed in this pull request?

Right now we are testing the most of CompactibleFileStreamLog in FileStreamSinkLogSuite (because FileStreamSinkLog once was the only subclass of CompactibleFileStreamLog, but now it's not the case any more).

Let's refactor the tests so that CompactibleFileStreamLog is directly tested, making future changes (like #15828, #15827) to CompactibleFileStreamLog much easier to test and much easier to review.

How was this patch tested?

the PR itself is about tests

@SparkQA
Copy link

SparkQA commented Nov 13, 2016

Test build #68580 has finished for PR 15870 at commit 3014a03.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class FakeCompactibleFileStreamLog(

@lw-lin
Copy link
Contributor Author

lw-lin commented Nov 13, 2016

To make it easier to review, this patch was made of two commits:

  • the first commit simply moved some test cases from one file to another; other than that nothing was touched;
  • the second commit did some refactor -- so it's suggested to review this second commit only

@zsxwing
Copy link
Member

zsxwing commented Nov 18, 2016

Thanks for doing this. Could you resolve the conflicts, please?

@lw-lin
Copy link
Contributor Author

lw-lin commented Nov 19, 2016

@zsxwing thanks. I'm rebasing right now.

@SparkQA
Copy link

SparkQA commented Nov 19, 2016

Test build #68879 has finished for PR 15870 at commit bb5ab33.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class CompactibleFileStreamLogSuite extends SparkFunSuite with SharedSQLContext
    • class FakeCompactibleFileStreamLog(

Copy link
Member

@zsxwing zsxwing left a comment

Choose a reason for hiding this comment

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

Looks good. Just one nit.

import java.nio.charset.StandardCharsets._

class CompactibleFileStreamLogSuite extends SparkFunSuite {
import scala.language.implicitConversions
Copy link
Member

Choose a reason for hiding this comment

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

nit: unused import

@SparkQA
Copy link

SparkQA commented Nov 22, 2016

Test build #68973 has finished for PR 15870 at commit 9d193d0.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@zsxwing
Copy link
Member

zsxwing commented Nov 22, 2016

LGTM. Thanks! Merging to master and 2.1.

@asfgit asfgit closed this in ebeb083 Nov 22, 2016
asfgit pushed a commit that referenced this pull request Nov 22, 2016
…mLog` directly

## What changes were proposed in this pull request?

Right now we are testing the most of `CompactibleFileStreamLog` in `FileStreamSinkLogSuite` (because `FileStreamSinkLog` once was the only subclass of `CompactibleFileStreamLog`, but now it's not the case any more).

Let's refactor the tests so that `CompactibleFileStreamLog` is directly tested, making future changes (like #15828, #15827) to `CompactibleFileStreamLog` much easier to test and much easier to review.

## How was this patch tested?

the PR itself is about tests

Author: Liwei Lin <lwlin7@gmail.com>

Closes #15870 from lw-lin/test-compact-1113.

(cherry picked from commit ebeb083)
Signed-off-by: Shixiong Zhu <shixiong@databricks.com>
robert3005 pushed a commit to palantir/spark that referenced this pull request Dec 2, 2016
…mLog` directly

## What changes were proposed in this pull request?

Right now we are testing the most of `CompactibleFileStreamLog` in `FileStreamSinkLogSuite` (because `FileStreamSinkLog` once was the only subclass of `CompactibleFileStreamLog`, but now it's not the case any more).

Let's refactor the tests so that `CompactibleFileStreamLog` is directly tested, making future changes (like apache#15828, apache#15827) to `CompactibleFileStreamLog` much easier to test and much easier to review.

## How was this patch tested?

the PR itself is about tests

Author: Liwei Lin <lwlin7@gmail.com>

Closes apache#15870 from lw-lin/test-compact-1113.
uzadude pushed a commit to uzadude/spark that referenced this pull request Jan 27, 2017
…mLog` directly

## What changes were proposed in this pull request?

Right now we are testing the most of `CompactibleFileStreamLog` in `FileStreamSinkLogSuite` (because `FileStreamSinkLog` once was the only subclass of `CompactibleFileStreamLog`, but now it's not the case any more).

Let's refactor the tests so that `CompactibleFileStreamLog` is directly tested, making future changes (like apache#15828, apache#15827) to `CompactibleFileStreamLog` much easier to test and much easier to review.

## How was this patch tested?

the PR itself is about tests

Author: Liwei Lin <lwlin7@gmail.com>

Closes apache#15870 from lw-lin/test-compact-1113.
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.

3 participants