Skip to content

Remove test duplication #133

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

Merged
merged 3 commits into from
Feb 15, 2017
Merged

Conversation

OlivierBlanvillain
Copy link
Contributor

This PR removes the test duplication introduced in #109.

As a defaults, new test should be added in shared/src/test. Parts that are JVM specific should go in jvm/src/test instead.

@SethTisue SethTisue mentioned this pull request Feb 6, 2017
val e: scala.xml.MetaData = Null //Node.NoAttributes
val sc: scala.xml.NamespaceBinding = TopScope
}

class XMLTest {
class XMLTestJVM {
import XMLTest.{ e, sc }
Copy link
Member

Choose a reason for hiding this comment

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

import XMLTestJVM... although why there's a companion object, I have no idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Force pushed the change.

…required

Moving jv tests to shared introduced name conflicts on the duplicated calsses, the rename solves that
@ashawley
Copy link
Member

ashawley commented Feb 9, 2017

I don't think there's any reason not to merge this. The duplication is indeed fixed. I'm sure there will be more re-work in the wake of these Scalajs changes.

@SethTisue
Copy link
Member

I'm going to go ahead and hit "merge" on this one since it may affect in-flight PRs.

@SethTisue SethTisue merged commit 822cdf3 into scala:master Feb 15, 2017
@OlivierBlanvillain OlivierBlanvillain deleted the deduplicate-tests branch February 15, 2017 23:30
@ashawley ashawley mentioned this pull request Oct 9, 2017
3 tasks
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