search

scala-js / scala-js-env-phantomjs

PhantomJS environment for Scala.js
BSD 3-Clause "New" or "Revised" License
2 stars 3 forks source link

Upgrade to Scala.js 1.0.0-M6. #23

Closed gzm0 closed 5 years ago

sjrd commented 5 years ago

I don't suppose you figured out what was going wrong here? Anything I can do to help?

gzm0 commented 5 years ago

I have no idea what's happening here. If we need this to announce M6, maybe you could have a look? I will probably not have time this week.

gzm0 commented 5 years ago

I have no idea what's happening here. If we need this to announce M6, maybe you could have a look? I will probably not have time this week.

gzm0 commented 5 years ago

I have no idea what's happening here. If we need this to announce M6, maybe you could have a look? I will probably not have time this week.

gzm0 commented 5 years ago

I have no idea what's happening here. If we need this to announce M6, maybe you could have a look? I will probably not have time this week.

gzm0 commented 5 years ago

I have no idea what is happening. If this is blocking M6, I'd be thankful if you could have a look. I do not expect to have time this week.

gzm0 commented 5 years ago

I have no idea what is happening. If this is blocking M6, I'd be thankful if you could have a look. I do not expect to have time this week.

gzm0 commented 5 years ago

I have no idea what is happening. If this is blocking M6, I'd be thankful if you could have a look. I do not expect to have time this week.

sjrd commented 5 years ago

I have nailed it down to:

The first bullet is fixed by changing this line https://github.com/scala-js/scala-js-env-phantomjs/blob/abc01c44963746dcd7d53e87e4f5b98d0128db2d/phantomjs-env/src/main/scala/org/scalajs/jsenv/phantomjs/ComRun.scala#L236 to use 3 instead of 2

The second bullet is more problematic. We have two choices:

In practice, I can make the tests pass if I modify largeMessageTest not to create unpaired surrogate values.

I'm not sure which is better. I would like to believe we should restrict to valid UTF-16 strings, but maybe that imposes some burden to JSEnvs users who would like to send "binary" messages encoded in strings.

gzm0 commented 5 years ago

I have no idea what is happening. If this is blocking M6, I'd be thankful if you could have a look. I do not expect to have time this week.

gzm0 commented 5 years ago

I'm not sure which is better. I would like to believe we should restrict to valid UTF-16 strings, but maybe that imposes some burden to JSEnvs users who would like to send "binary" messages encoded in strings.

Binary messages should be encoded in ASCII strings, so specifying that the strings need to be valid UTF-16 is fine IMO.

sjrd commented 5 years ago

OK, so then the test largeMessageTest as such is broken. I'm not sure how I can convince JUnit to ignore that one test, so that we can have a passing build to release PhantomJS for 1.0.0-M6.

sjrd commented 5 years ago

So, since I couldn't find a way to ignore only largeMessageTest, I just disabled the whole suite so that the CI would be happy :( Locally, every test passes except largeMessageTest.

Because of GitHub's problems today, the status isn't reported on the PR (currently), but the build is green at https://travis-ci.org/scala-js/scala-js-env-phantomjs/builds/444766920

@gzm0 How do you feel about that?

gzm0 commented 5 years ago

Have you considered implementing your own runner so you can apply a filter?

See: https://junit.org/junit4/javadoc/4.12/org/junit/runners/ParentRunner.html

sjrd commented 5 years ago

I had not thought about that. That is some advanced JUnit-fu! I think I managed to do it. See latest commit.

sjrd commented 5 years ago

Updated, with the simplification and squashed commits.

gzm0 commented 5 years ago

LGTM