Add tests for game.Event

[lenguyenthanh]

To prevent: https://github.com/lichess-org/lila/pull/15699, We should add more tests to cover game.Event, example: #15710

[darthvader3010]

Hi i would like to try this!

[lenguyenthanh]

Hi i would like to try this!

go for it!

[lenguyenthanh]

Hi i would like to try this!

hi, how is this going? We need this the sooner the better, so if you have any issue don't hesitate to ask question here or on lidiscord.

[sam-mao-29]

Hey, I'd like to give this a shot. Also apologies if this is a silly question, but I'm new to contributing to lichess and on gitpod - and I'm not quite sure how to run tests from the terminal, or via some sort of extension available. I tried running 'sbt test' in the terminal but got 'bash: sbt: command not found'.

[fitztrev]

on gitpod - and I'm not quite sure how to run tests from the terminal

on gitpod, you'll need to run the tests from within a container:

docker compose run --rm -w /lila --entrypoint="sbt test" lila

[nicvagn]

Howdy.

I do not pretend to be good at Scala, just learning. Just so I have a better understanding of the problem statement.

  case class Promotion(role: PromotableRole, pos: Square) extends Event:
    def typ = "promotion"
    def data =
      Json.obj(
        "key"        -> pos.key,
        "pieceClass" -> role.toString.toLowerCase
      )

I do not see anyway to meaningfully test the Promotion case class. Is that correct?

I could however test:

case class EndData(game: Game, ratingDiff: Option[chess.ByColor[IntRatingDiff]]) extends Event:
    def typ = "endData"
    def data =
      Json
        .obj(
          "winner" -> game.winnerColor,
          "status" -> game.status
        )
        .add("clock" -> game.clock.map: c =>
          Json.obj(
            "wc" -> c.remainingTime(Color.White).centis,
            "bc" -> c.remainingTime(Color.Black).centis
          ))
        .add("ratingDiff" -> ratingDiff.map: rds =>
          Json.obj(
            Color.White.name -> rds.white,
            Color.Black.name -> rds.black
          ))
        .add("boosted" -> game.boosted)```

        by feeding it a "case" game and seeing if the output is as expected?

        for ```  case class Clock(white: Centis, black: Centis, nextLagComp: Option[Centis] = None) extends ClockEvent:
    def typ = "clock"
    def data =
      Json
        .obj(
          "white" -> white.toSeconds,
          "black" -> black.toSeconds
        )
        .add("lag" -> nextLagComp.filter(_ > 1))
  object Clock:
    def apply(clock: ChessClock): Clock =
      Clock(
        clock.remainingTime(Color.White),
        clock.remainingTime(Color.Black),
        clock.lagCompEstimate(clock.color)
      )

However, my knowledge of the code base (and FP) is not sufficient to properly create adequate tests here.

 case class CheckCount(white: Int, black: Int) extends Event:
    def typ = "checkCount"
    def data =
      Json.obj(
        "white" -> white,
        "black" -> black
      )

How would one go about creating test's for case classes?

I see them as being descriptive not taking an action?

I looked at your example as to what you wanted to avoid, however I did not understand it.

Thank you for reading.

[nicvagn]

I think I may be not comprehending correctly what I am to test...

Sorry, If it is not a burden, could you be more explicit in what you desire test's to be written for?

I interpreted testing game.Event for writing testing for the code in lila/modules/game/src/Event.scala

I am growing to believe this incorrect.

[lenguyenthanh]

Howdy.

I do not pretend to be good at Scala, just learning. Just so I have a better understanding of the problem statement.

hey, We all started as beginners, thanks for interested in the issue!

  case class Promotion(role: PromotableRole, pos: Square) extends Event:
    def typ = "promotion"
    def data =
      Json.obj(
        "key"        -> pos.key,
        "pieceClass" -> role.toString.toLowerCase
      )

I do not see anyway to meaningfully test the Promotion case class. Is that correct?

As I written in the issue, the motivation is avoid regression. This game.Event class is a protocol between clients and server, if We did something stupid (like I did: https://github.com/lichess-org/lila/pull/15699), it'll be terrible.

I looked at your example as to what you wanted to avoid, however I did not understand it.

My pr in the example has two kind of tests, example based tests (or unit test) and property based tests. You can just ignore the latter and focus on unit testing: https://github.com/lichess-org/lila/pull/15710/files#diff-94771845f4513b49c03b971fec30f5fd9439dbccac076ec4b7fce295e569614dR13. for example (pseudo code)

val promotion: Promotion = ??? (some specific value) val correctOuput = ??? assert(promotion.data, correctOutput)

Thank you for reading.

You're welcome and don't hesitate to ask more questions. I hope my answer helps but I'm on my phone so maybe I have some details wrong.

[nicvagn]

Thanks, much of my confusion has been eliminated.

However, one particular term still causes me some confusion.

What do you mean by regression?

This is a new term for me in the context of testing.

(I found this, but it was not illuminating)[https://spark.apache.org/docs/3.1.1/api/scala/org/apache/spark/ml/regression/LinearRegression.html]

[nicvagn]

So here

All I see is going from a complicated, error prone implementation to a simple maintainable one.

If there are obvious error's in the earlier implementation I am not knowledgeable enough to spot them.

I see how the new implementation is better, however I do not fully understand how testing would prevent this

[lenguyenthanh]

What do you mean by regression?

This is a new term for me in the context of testing.

I mean this: https://en.wikipedia.org/wiki/Regression_testing

But basically, We want to guard us from unwanted change in our protocol.

[lenguyenthanh]

All I see is going from a complicated, error prone implementation to a simple maintainable one.

If there are obvious error's in the earlier implementation I am not knowledgeable enough to spot them.

yes, it's hard to spot the error. The implementations of these two are almost identical. But the output of stringFromPossibleMoves are different, that caused a lot of trouble.

I see how the new implementation is better, however I do not fully understand how testing would prevent this

The test here: https://github.com/lichess-org/lila/pull/15710/files#diff-94771845f4513b49c03b971fec30f5fd9439dbccac076ec4b7fce295e569614dR13-R17 will prevent this happen again, because if output of stringFromPossibleMoves changed the test will failed.

[nicvagn]

Thank you for illuminating this for me, I understand better.

[lenguyenthanh]

no worries and again don't hesitate to ask more questions.

[nicvagn]

  test("EndData anti Reggression."):
    val game            = ??? // chess.Game(chess.Board())
    val ratingDiff      = ???
    val endData         = Event.EndData(game, ratingDiff)
    val expectedEndData = ???
    assertEquals(endData, expectedEndData)

Is this starting down the correct path?

[lenguyenthanh]

looks right to me

[nicvagn]

thanks,

I have some confusion about creating the "test" known good values. At the current time, I am trying to create the Game object.

There is core.Game game object and game.Game case class. and creating a Game object is proving a challenge.

Is there a "factory" or such to create game objects?

[nicvagn]

I need to go back to the books... Thank you for your help, but I think this is beyond my ability as of the moment.

[lenguyenthanh]

I need to go back to the books... Thank you for your help, but I think this is beyond my ability as of the moment.

hey, no worry, You probably picked the hardest thing to test (EndData). And I probaly underestimate the difficulty of this one. it's definitely not a good first issue.

[nicvagn]

I need to go back to the books... Thank you for your help, but I think this is beyond my ability as of the moment.

hey, no worry, You probably picked the hardest thing to test (EndData). And I probaly underestimate the difficulty of this one. it's definitely not a good first issue.

Thanks for your help regardless. I apologize for not bearing fruit.

It is often difficult to see the intricacies of a problem from a layman's perspective.

I would really like to see the conclusion of this issue.

Good night (or day) cheers.