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 interface #755

Merged
merged 22 commits into from Jun 14, 2017

Conversation

Projects
None yet
4 participants
@Duhemm
Collaborator

Duhemm commented May 29, 2017

This is a super pre alpha version of the test interface. I'm opening the PR for people who may want to play with it, and look at what it may look like in the future. It still has quite a few issues. It's certainly not worth doing a full review at the moment.

If you want to try it out, you'll need to:

It follows the design of the test interface of ScalaJS, but there are a few differences:

  • The distinction between master and slave is still half there. The distinction is not observed at the moment, as it is not clear to me why this is needed.
  • We don't serialise the task, but keep them on the remote runner and refer to them using an ID that is assigned.
  • We have a different serialisation.

@Duhemm Duhemm force-pushed the Duhemm:topic/test-interface branch from cca46ac to 3aa375c May 29, 2017

@muxanick

This comment has been minimized.

Contributor

muxanick commented May 29, 2017

@Duhemm great work, do you mind to move sockets to separate PR? I would like to add windows port

@Duhemm

This comment has been minimized.

Collaborator

Duhemm commented May 30, 2017

@muxanick I just opened #758

@Duhemm

This comment has been minimized.

Collaborator

Duhemm commented May 30, 2017

I just tested, it works nice with uTest and scalacheck (modulo fixing a compiler bug for scalacheck):

> sandbox/test
[info] Linking (1937 ms)
[info] Discovered 1847 classes and 12207 methods
[info] Optimizing (1503 ms)
[info] Generating intermediate code (403 ms)
[info] Produced 61 files
[info] Compiling to native code (2271 ms)
[info] Linking native code (312 ms)
[info] Starting process '/Users/martin/Documents/Projects.nosync/Duhemm/scala-native/sandbox/target/scala-2.11/sandbox-out' on port '9841'.
[info] Client connected with IP address: 0.0.0.0
[info] Starting process '/Users/martin/Documents/Projects.nosync/Duhemm/scala-native/sandbox/target/scala-2.11/sandbox-out' on port '9363'.
[info] Client connected with IP address: 0.0.0.0
[info] -------------------------Starting Suite foobar.MoreTest-------------------------
[info] foobar.MoreTest.test2            Success
[info] foobar.MoreTest          Success
[info] ------------------------Starting Suite foobar.UTestTest------------------------
[info] Printing something from test...
[info] foobar.UTestTest.test1           Success
[info] foobar.UTestTest         Success
[info] + String.startsWith: OK, passed 100 tests.
[info] ScalaCheck
[info] Passed: Total 1, Failed 0, Errors 0, Passed 1
[info] utest
[info] -----------------------------------Results-----------------------------------
[info] foobar.UTestTest         Success
[info]     test1                Success
[info] foobar.MoreTest          Success
[info]     test2                Success
[info]
[info] Tests: 4
[info] Passed: 4
[info] Failed: 0
[info] Passed: Total 0, Failed 0, Errors 0, Passed 0
@densh

This comment has been minimized.

Member

densh commented Jun 7, 2017

Needs a rebase due to conflicting changes in .travis.yml

@Duhemm Duhemm force-pushed the Duhemm:topic/test-interface branch 2 times, most recently from 6fea41f to b3f7657 Jun 8, 2017

@densh

This comment has been minimized.

Member

densh commented Jun 12, 2017

@Duhemm Sockets are merged now, please rebase on top of latest master to resolve the merge conflict.

@Duhemm Duhemm force-pushed the Duhemm:topic/test-interface branch from b3f7657 to ba23afb Jun 12, 2017

@Duhemm

This comment has been minimized.

Collaborator

Duhemm commented Jun 12, 2017

@densh I've rebased it. It still includes everything to clone and build my forks of utest and scalacheck. Let me know when you think this PR is ready so that I can remove all of that.

@Duhemm Duhemm changed the title from [WIP] Test interface to Test interface Jun 12, 2017

@densh

First mini-pass.

.travis.yml Outdated
- mkdir utest
- pushd utest
- git init .
- git remote add -t cross-native Duhemm https://github.com/Duhemm/utest.git

This comment has been minimized.

@densh

densh Jun 12, 2017

Member

For the testing frameworks: they should depend on the test interface artifact, not the stubs artifact.

This comment has been minimized.

@densh

densh Jun 12, 2017

Member

Also, once this PR is merged we should upstream changes made to utest and scalacheck.

This comment has been minimized.

@densh

densh Jun 12, 2017

Member

Btw, sbt-crossproject is now at 0.2.0, we released newer version but haven't announced it yet.

build.sbt Outdated
libraryDependencies -= "org.scala-native" %%% "test-interface" % version.value % Test
)
.enablePlugins(ScalaNativePlugin)
.dependsOn(testInterfaceSerialization, stubs)

This comment has been minimized.

@densh

densh Jun 12, 2017

Member

Do we really need to split test interface and stubs sub projects? Can we merge them together?

/** A `ClassLoader` that returns pre-instanciated objects.
* We use it to bypass uses of reflection in the test interface.
*/
class PreLoadedClassLoader(map: Map[String, AnyRef]) extends ClassLoader {

This comment has been minimized.

@densh

densh Jun 12, 2017

Member

Given that preload is a single word, the name should be PreloadedClassLoader.

def serialize[T: Serializable](v: T): Iterator[String] =
implicitly[Serializable[T]].serialize(v)
def deserialize[T: Serializable](in: Iterator[String]): T =
implicitly[Serializable[T]].deserialize(in)

This comment has been minimized.

@densh

densh Jun 12, 2017

Member

Given that serialization here isn't meant to be extensible, it's better to go for lower-level abstraction-free approach here. Current implementation is probably rather slow as it relies on virtual dispatch heavily.

First of all type-classes are an overkill here due to the fact that all instances are statically known and we don't expect any extensibility beyond that point.

Second, for serialization it's best to have a mutable builder-like code to minimize I/O overhead. Please have a look at NIR serialization and Show builder for examples. We used to do high-level type-class like code and it was very expensive, even on the JVM (2-3x performance overhead.)

@densh

Second mini-pass.

@@ -43,6 +45,9 @@ final class _Class[A](val ty: Ptr[Type]) {
def getSuperclass(): Class[_ >: A] =
???
def getField(name: String): Field =
null

This comment has been minimized.

@densh

densh Jun 12, 2017

Member

Why does this one return null and not ??? ?

It might give a false impression of an implemented API, but we don't support this at the moment.

package java.lang.reflect
class Field {
def get(obj: Object): Object = null

This comment has been minimized.

@densh

densh Jun 12, 2017

Member

The same problem as for Class::getField, it should throw via ??? given that it's not implemented.

@@ -0,0 +1,6 @@
package scala.scalajs.js.annotation
class JSExportDescendentObjects(ignoreInvalidDescendants: Boolean)

This comment has been minimized.

@densh

densh Jun 12, 2017

Member

This is one is deprecated in Scala.js and we should keep it being deprecated in native.

@@ -0,0 +1,6 @@
package scala.scalajs.js.annotation
class JSExportDescendentClasses(ignoreInvalidDescendants: Boolean)

This comment has been minimized.

@densh

densh Jun 12, 2017

Member

This is one is deprecated in Scala.js and we should keep it being deprecated in native.

val server_address = native.alloc[sockaddr_in]
!server_address._1 = socket.AF_INET.toUShort
!server_address._2 = htons(port)
!server_address._3._1 = htonl(INADDR_ANY)

This comment has been minimized.

@densh

densh Jun 12, 2017

Member

sockaddr and friends badly need to have implicit classes that alias their field names to their human-readable counter-parts.

@Duhemm

This comment has been minimized.

Collaborator

Duhemm commented Jun 13, 2017

@densh I've address all of your comments.

@densh

Third pass.

@@ -115,3 +115,36 @@ object in {
def IN6_IS_ADDR_MC_GLOBAL(arg: Ptr[in6_addr]): CInt = extern
}
object inh {

This comment has been minimized.

@densh

densh Jun 14, 2017

Member

We decided to go for foobarOps instead of foobarh for clarity.

@@ -168,3 +168,16 @@ object socket {
length: CSize,
flags: CInt): CSSize = extern
}
object socketh {

This comment has been minimized.

@densh

densh Jun 14, 2017

Member

The same as above: socketOps

}
/** Wait for a message to arrive from the distant program. */
def receive[T: Serializable](timeout: Duration = Duration.Inf): T =
def receive[T](timeout: Duration = Duration.Inf): Message =

This comment has been minimized.

@densh

densh Jun 14, 2017

Member

Is type parameter T used anywhere?

@densh

One more thing.

.travis.yml Outdated
- git checkout Duhemm/cross-native
- sbt publishLocal
- popd
- bash sbt -J-Xmx2G "set scriptedBufferLog in sbtScalaNative := false" sandbox/test test-all

This comment has been minimized.

@densh

densh Jun 14, 2017

Member

As soon as we remove temporary dependency on your branch of utest and scalacheck, we'll have no tests whatsoever for the testing framework integration. I suggest that we migrate our current mini-framework we used for tests project to the new testing framework integration to address this.

@Duhemm Duhemm force-pushed the Duhemm:topic/test-interface branch from 378f11e to 7a2745e Jun 14, 2017

@densh

See minor comment below, also needs a rebase due to diverging changes on BufferedWriterSuite.scala.

//
// exit(if (success) 0 else 1)
// }
//}

This comment has been minimized.

@densh

densh Jun 14, 2017

Member

This file is not necessary any longer.

@Duhemm Duhemm force-pushed the Duhemm:topic/test-interface branch from 3936279 to 1f31ffd Jun 14, 2017

@Duhemm

This comment has been minimized.

Collaborator

Duhemm commented Jun 14, 2017

@densh Done!

@densh

This comment has been minimized.

Member

densh commented Jun 14, 2017

Another conflicting change due to PR merged to master on FileInputStreamSuite.scala...

Duhemm added some commits May 19, 2017

Add initial version of the test interface
There are still many rough edges and issues with it. See comments in the
code for the different issues to tackle :)
Fix how tests are instantiated
We were assuming that tests are always inside objects, but that's not
true. Fortunately, fingerprints tell us all we need to instantiate the
tests.

@Duhemm Duhemm force-pushed the Duhemm:topic/test-interface branch from 1f31ffd to ffdecf4 Jun 14, 2017

@Duhemm

This comment has been minimized.

Collaborator

Duhemm commented Jun 14, 2017

@densh Done!

@densh

densh approved these changes Jun 14, 2017

@densh

This comment has been minimized.

Member

densh commented Jun 14, 2017

As soon as CI passes, this is LGTM! Fantastic work @Duhemm! 🍰

@densh densh referenced this pull request Jun 14, 2017

Closed

Add support for sbt testing interface #339

0 of 3 tasks complete

@densh densh merged commit 074ff0e into scala-native:master Jun 14, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
case sf: SubclassFingerprint => sf.isModule
}
val inst = if (isModule) t.name else s"new ${t.name}"

This comment has been minimized.

@xuwei-k

xuwei-k Jun 16, 2017

Contributor
val inst = if (isModule) s"_root_.${t.name}" else s"new _root_.${t.name}"
s""""${t.name}" -> $inst"""

rickynils/scalacheck#337 (comment)

asoltysik added a commit to asoltysik/scala-native that referenced this pull request Jul 14, 2017

Test interface (scala-native#755)
* Make `StackTraceElement` closer to Java's impl

* Add initial version of the test interface

There are still many rough edges and issues with it. See comments in the
code for the different issues to tackle :)

* Teach Travis how to build scalacheck

* scalafmt

* Cleanup

* Remove debug info from serializers

* Fix how tests are instantiated

We were assuming that tests are always inside objects, but that's not
true. Fortunately, fingerprints tell us all we need to instantiate the
tests.

* Don't do `rebuild` twice in Travis CI

* scalafmt

* Correctly convert variants of `struct sockaddr`

Previously we would just copy a pre-defined number of bytes, now matter
what the actual underlying struct was. Now, we use the length of the
input struct to determine its type, and correctly convert to the
operating system implementation, so that no information is lost.

* %s/PreLoaded/Preloaded/g

* Remove `stubs` project

* Remove impls in `Class.getField` and `Field.get`

* Deprecate `JSExportDescendent{Classes,Objects}`

* Simpler serialization

* Add implicit classes for `sockaddr` and friends

* Give names to fields of structs for sockets

* Fix review comments

* Prefix test name with `_root_.`

* Add NativeFramework to run our unit tests

* Remove utest and scalacheck tests

* Remove old, dead code

@Duhemm Duhemm deleted the Duhemm:topic/test-interface branch Sep 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment