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.

Sign up for GitHub

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

Jump to bottom

[SLP-0047] Fix build errors of issue SLI-0046 #47

Merged
merged 5 commits into from
Apr 3, 2020
Merged

[SLP-0047] Fix build errors of issue SLI-0046 #47

merged 5 commits into from
Apr 3, 2020

Conversation

alejgh
Copy link
Member

@alejgh alejgh commented Apr 3, 2020 •
edited
Loading

Work done:

  • I have restored the sbt version to 1.3.7. With that the following error is fixed: The compiler bridge sources org.scala-sbt:compiler-bridge_2.13:1.1.1:compile could not be retrieved.
  • I have modified the project to be compatible with both Scala 2.12.10 and 2.13.1. This allows us to make two different Travis builds, one when the coverage is run (2.12.10) and the other one where just tests are run (2.13.1)
  • I have submitted a temporal fix for the error where one test would fail when executing tests from the command line. The problem comes from the MemorySymbolTableUnitTest class. When running the test suite the tests from this class are executed before the IdentificationWalker tests, and the state of the MemoryErrorHandler was not restored, containing an error from the MemorySymbolTable tests. In theory, the state should be restored with the before method, but that was not the case. For now I substituted the before method for an after one, so after each test in the MemorySymbolTable suite the state is restored. This works right now, but we should open a new issue to look a the cause of this problem and propose a better solution.

Closes #46

thewillyhuman
thewillyhuman reviewed Apr 3, 2020
View reviewed changes
test/unit/compiler/semantic/IdentificationWalkerTest.scala
@@ -33,11 +33,8 @@ class IdentificationWalkerTest extends AnyFunSuite with BeforeAndAfter {
final val logger = Logger[IdentificationWalkerTest]

// In order to be sure that on each test case we do not have data from previous tests.
before {
logger.debug("Restoring memory symbol table.")
Copy link
Member

Choose a reason for hiding this comment

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

What happened to the logger?

Copy link
Member Author

Choose a reason for hiding this comment

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

I restored the logging messages in the following commit, my bad.

test/unit/compiler/MemorySymbolTableUnitTest.scala
@@ -48,7 +48,7 @@ class MemorySymbolTableUnitTest extends AnyFunSuite with BeforeAndAfter {
val shape = new ShapeDeclaration("example", 4,1,
new PrefixInvocation("example", 4,2, "this is my shape name", prefix), null)

before {
after {
Copy link
Member

Choose a reason for hiding this comment

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

Was any problem with the original before impl?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, I will document everything later on in the issue, this is still WIP

Copy link
Member

Choose a reason for hiding this comment

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

okay thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

I have updated the comment

@thewillyhuman thewillyhuman linked an issue Apr 3, 2020 that may be closed by this pull request
[SLI-0046] Build status of master is failing #46
Closed
@thewillyhuman thewillyhuman added this to the ShEx Lite - 2.0 milestone Apr 3, 2020
@alejgh
Copy link
Member Author

alejgh commented Apr 3, 2020

Right now the only thing that bothers me is that Travis is making a build just for Scala 2.13.1 and not for both 2.13.1 and 2.12.10. Do you know what could be the problem?

@alejgh
Copy link
Member Author

alejgh commented Apr 3, 2020

I would like to check the build with 2.12.10 (where the coverageReport is done) before merging this.

@thewillyhuman
Copy link
Member

I would like to check the build with 2.12.10 (where the coverageReport is done) before merging this.

Yes I've been looking the travis generated logs and it is skiping the 2.12.10 version.. Would like to fix it before merging too.

@alejgh
Copy link
Member Author

alejgh commented Apr 3, 2020

I think I got it, the problem is that we are using the "matrix" functionality to run a Python build too. I separated the 2.12.10 and 2.13.1 builds, lets see if it works.

@thewillyhuman
Copy link
Member

If it works you can have the rest of the week free >.< (It's friday)

@alejgh
Copy link
Member Author

alejgh commented Apr 3, 2020

If it works you can have the rest of the week free >.< (It's friday)

Nah, the 2.12.10 build is failing. This is going to be a tough one...

@codecov
Copy link

codecov bot commented Apr 3, 2020 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (shex-lite-2.0-dev@41e4a7d). Click here to learn what that means.
The diff coverage is 50%.

Impacted file tree graph

@@                 Coverage Diff                  @@
##             shex-lite-2.0-dev      #47   +/-   ##
====================================================
  Coverage                     ?   41.55%           
====================================================
  Files                        ?        7           
  Lines                        ?      219           
  Branches                     ?       19           
====================================================
  Hits                         ?       91           
  Misses                       ?      128           
  Partials                     ?        0
Impacted Files Coverage Δ
src/compiler/syntactic/ShExLSyntacticParser.scala 22.58% <ø> (ø)
src/compiler/semantic/MemoryErrorHandler.scala 76.92% <50%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41e4a7d...928f5b3. Read the comment docs.

@alejgh
Copy link
Member Author

alejgh commented Apr 3, 2020

Seems to be working now.

@alejgh alejgh requested a review from thewillyhuman April 3, 2020 13:16
@alejgh alejgh removed the status/wip label Apr 3, 2020
@alejgh alejgh changed the title [WIP] Fix build errors of issue #46 Fix build errors of issue #46 Apr 3, 2020
@thewillyhuman thewillyhuman merged commit 0356b0e into shex-lite-2.0-dev Apr 3, 2020
@thewillyhuman thewillyhuman deleted the fix_issue_46 branch April 3, 2020 13:54
@thewillyhuman thewillyhuman changed the title Fix build errors of issue #46 [SLP-0047] Fix build errors of issue SLI-0046 Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thewillyhuman thewillyhuman Awaiting requested review from thewillyhuman

Assignees
No one assigned
Labels
affects/repository
Projects
None yet
Milestone
shex-lite-2.0-beta
Development

Successfully merging this pull request may close these issues.

[SLI-0046] Build status of master is failing
2 participants
@alejgh @thewillyhuman