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

Sbt 1.0 and version upgrades, scala.js test fixes #333

Open
wants to merge 4 commits into
base: series/0.8.x
Choose a base branch
from

Conversation

bastiion
Copy link

@bastiion bastiion commented Dec 1, 2017

While working on a jsonld.js feature I recognized that none of the scala.js code was actually beeing tested. There were some mistakes in the way how the jsonld.js resource was beeing loaded (it coudln't be referenced in an node.js environment because you have to specify it commonJSName).
Furthermore dependencies of scala.js crossprojects have to be specified with "org.organisation" %%% "package" % "0.0" this cannot not be done in Dependencies.scala.

In general I thought it was time to leverage the build chain, to use the latest sbt version and the plugin-dependencies.

Unfortunatly I coudln't get unidoc working at the moment. Further investigation is necessary, what needs to be aggregated and excluded.

 - fixed all things, that preveted tests on node.js from running
 - unidoc currently not working (needs configuration)
 - scalazVersion and ScalatestVersion to refer to these libraries with %%%
 - version bumps of scalaz, scalatest and akkaHttpCore
- with minor modifications to allow loading in node.js testing
  environment and in browser test
- master branch of http://github.com/digitalbazaar/jsonld.js (commit id 3f0ad206db4e863222765e4289fe756b3f80d753)
@@ -44,13 +44,13 @@ object jsonldHelper {
}

@js.native
@JSGlobal
Copy link
Author

Choose a reason for hiding this comment

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

This will be mandadory in scala.js-1.0


import scalajs.concurrent.JSExecutionContext.Implicits.queue
import org.w3.banana.plantain._

object JsonLdJsParserTest extends WordSpec with Matchers {
class JsonLdJsParserTest extends WordSpec with Matchers {
Copy link
Author

Choose a reason for hiding this comment

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

object test-suites are not executed by scalatest in js-environments

).graph
-- schema("name") ->- "Manu Sporny"
-- schema("url") ->- URI("http://manu.sporny.org/")
-- schema("image") ->- URI("http://manu.sporny.org/images/manu.png")).graph

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure the above is an improvement...

Copy link
Author

Choose a reason for hiding this comment

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

this is the result of scalariform run, we can omit that (maybe we have to tune default settings?)

@@ -1,13 +1,13 @@
package org.w3.banana
package jsonldjs

import org.scalatest.{Matchers, WordSpec}
import org.scalatest.{ Matchers, WordSpec }
Copy link
Member

Choose a reason for hiding this comment

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

I was looking for the style guidelines to check if we really had decided to put spaces after the squigly brackets... I would not otherwise know to decide which was better.

@@ -88,7 +90,7 @@ lazy val rdf = crossProject
.settings(commonSettings: _*)
.settings(
name := "banana-rdf",
libraryDependencies += scalaz
libraryDependencies += "org.scalaz" %%% "scalaz-core" % scalazVersion
Copy link
Member

@bblfish bblfish Dec 6, 2017

Choose a reason for hiding this comment

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

why do you remove the scalaz val defined in project/Dependencies.scala?

Copy link
Author

Choose a reason for hiding this comment

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

see my comment below

@@ -112,7 +114,7 @@ lazy val rdfTestSuite = crossProject
.settings(commonSettings: _*)
.settings(
name := "banana-test",
libraryDependencies += scalatest,
libraryDependencies += "org.scalatest" %%% "scalatest" % scalatestVersion,
Copy link
Member

Choose a reason for hiding this comment

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

why this change? The scalatest val is still available in project/Dependencies.scala as far as I can see reading the web view here.

Copy link
Author

Choose a reason for hiding this comment

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

It is because in Dependencies.scala there is a statement:

Note: %%% can only be used within a task or setting macro, such as :=, +=, ++=, Def.task, or Def.setting...

Yesterday I read at the scala-js gitter channel:

Ah, in a .scala file you need an additional import: import org.portablescala.sbtplatformdeps.PlatformDepsPlugin.autoImport._

I already tested but it is still not working stating:

Dependencies.scala:33:47: value %%% is not a member of String

This is one of the root causes, that tests aren't running and scala-js dependecies are not set correctly (which is especially bad if a library wants to use banana-rdf-sjs without manually adding scalaz)

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

The %%% method is defined there, with private[sbtplatformdeps] in its parent class signature.

https://github.com/portable-scala/sbt-platform-deps/blob/master/src/main/scala/org/portablescala/sbtplatformdeps/PlatformDepsBuilders.scala

Until we know how to declare it in project/Dependecies.scala, IMHO we should take the imperfect solution, as the current release of banana-rdf can be considered broken - sjs-libs are neither tested nor are they published with correct dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

ok, just open an issue and link to it, in the dependencies with the explanation. Then it will be easier to remember to do something when the right time comes.

lazy val jsonldJS = Project("jsonld", file("jsonld.js"), settings = commonSettings)
lazy val jsonldJS = Project("jsonld", file("jsonld.js"))
.settings(commonSettings: _*)
.settings()
Copy link
Member

Choose a reason for hiding this comment

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

is this empty settings doing any work?

Copy link
Author

Choose a reason for hiding this comment

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

no, this is a mistake

Copy link
Member

Choose a reason for hiding this comment

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

ok, should be fixed then for the PR to go through :-)

@bblfish
Copy link
Member

bblfish commented Sep 4, 2020

Hi @bastiion have the lastest big PRs addresses the issues that you described in here? Should I close it?

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.

None yet

2 participants