cucumber / cucumber-jvm

Cucumber for the JVM
https://cucumber.io
MIT License
2.7k stars 2.02k forks source link

Transformer doesn't convert "[empty]" values in the table to empty string ("") #2262

Closed manish7-thakur closed 3 years ago

manish7-thakur commented 3 years ago

Describe the bug Updated to version 6.10.0 recently and went through the documentation https://github.com/cucumber/cucumber-jvm-scala/blob/main/docs/transformers.md but seems like none of the registered transformers for empty values in being used during the transformation. Not sure at what stage the transformer is called or if I'm doing something wrong.

Registered the transformer defined as:

  DefaultDataTableCellTransformer("[empty]")((s: String, t: java.lang.reflect.Type) => {
    new StringBuilder().append(s).append("-").append(t)
  })

and can see it's being registered (see attached screenshot) with glue but after transformation the DataTable values are still coming out to be [empty].

To Reproduce Steps to reproduce the behavior.

  Scenario: Test case 2, hotel with special condition
    Given Hotels 419 checkin 2020-10-01 los 2
    And BookingDate 2020-09-19 01:30:00
    And GMTOffset +7
    When The user search
    Then The cancellation policy from search result should match following expected value
      | RoomId     | Promotion | Description | DefaultDescription | FreeCancellationDate          | Title               | Symbol            |
      | 1199923528 | -         |  [empty]    |       [empty]      | 2020-09-19T00:00:00.000+07:00 | Cancellation policy | special-condition |

Step is defined as:

  Then("""^The search result should match following expected value$""") { (dt: DataTable) =>
    val result = parseResult(response, dataExtractors)
    validateResult(dt, result)
  }

Expected behavior I expected the cells in features marked with [empty would be converted to "" against which result can be validated.

Versions:

Additional context Add any other context about the problem here.

Screen Shot 2021-03-11 at 14 26 04

gaeljw commented 3 years ago

Hi @manish7-thakur, can you provide us the validateResult method or at least how you use the dt: DataTable parameter?

One thing to note though is that DefaultDataTableCellTransformer is used to convert from a String to a type t, in your implementation you are always returning a StringBuilder. This kinda works but is not the intended usage.

TBH I've rarely see any use of the DefaultDataTableCellTransformer, most of the time DataTableType fits better but I would need to see your DataTable usage to understand what you are trying to do in the end.

mpkorstanje commented 3 years ago

One thing to note though is that DefaultDataTableCellTransformer is used to convert from a String to a type t, in your implementation you are always returning a StringBuilder. This kinda works but is not the intended usage.

For the [empty] replacement to work as expected the return type should be String.

gaeljw commented 3 years ago

For the [empty] replacement to work as expected the return type should be String.

Right, didn't think about that! Thanks @mpkorstanje

manish7-thakur commented 3 years ago

Hi @manish7-thakur, can you provide us the validateResult method or at least how you use the dt: DataTable parameter?

One thing to note though is that DefaultDataTableCellTransformer is used to convert from a String to a type t, in your implementation you are always returning a StringBuilder. This kinda works but is not the intended usage.

TBH I've rarely see any use of the DefaultDataTableCellTransformer, most of the time DataTableType fits better but I would need to see your DataTable usage to understand what you are trying to do in the end.

@gaeljw validateResult is just used to find table diff after prepending header to actual response. I expect that at this point the expected datable's [empty] cells would be replaced with string values.

  def validateResult(expected: DataTable, actual: Seq[String], sameSorting: Boolean = false): Unit = {
    val rows = actual.map(d => d.tail.split("\\|"))
    val header = if (!expected.cells().isEmpty) expected.cells().get(0).asScala.toArray else Array.empty[String]
    val data = if (!header.isEmpty || rows.nonEmpty) DataTable.create((header :: rows.toList).map(_.toList.asJava).asJava) else ValidationResult.EmptyExpectedResult

    try if (sameSorting) expected.diff(data) else expected.unorderedDiff(data)
    catch {
      // To avoid stack trace on the HTML report
      case tde: TableDiffException =>
        throw CustomDiffException(tde.toString)
      case e: Exception => throw e
    }
  }
manish7-thakur commented 3 years ago

For the [empty] replacement to work as expected the return type should be String.

Right, didn't think about that! Thanks @mpkorstanje

Well I have tried String and other response types. Even tried the JacksonDefaultDataTableEntryTransformer. https://github.com/cucumber/cucumber-jvm-scala/blob/73c33a0acc3185af74a5a34e55436acc64310a4a/cucumber-scala/src/main/scala/io/cucumber/scala/JacksonDefaultDataTableEntryTransformer.scala

But nothing seems to work out. If you say that return type should be String, then how come this test works: https://github.com/cucumber/cucumber-jvm-scala/blob/e584a27ffb090ac562ba6e6a7a19e8bba6e31ea3/cucumber-scala/src/test/scala/io/cucumber/scala/ScalaDslDefaultDataTableEntryTransformerTest.scala#L78

At the moment I have to work around this issue with:

  private def replaceWithEmptyString(expected: DataTable) = {
    val enrichedCells = expected.cells().asScala.map(_.asScala.map {
      case "[empty]" => ""
      case str => str
    }.asJava).asJava
    DataTable.create(enrichedCells)
  }
gaeljw commented 3 years ago

I would need to check the code of Cucumber core to be 100% sure but I believe transformers are not applied when you access the DataTable directly with the cells() method.

Transformers are applied when you use for instance:

dataTable.asLists[String](classOf[String]) // java.util.List[java.util.List[String]]
// Or
dataTable.asScalaRawLists[String] // Seq[Seq[String]]
manish7-thakur commented 3 years ago

I would need to check the code of Cucumber core to be 100% sure but I believe transformers are not applied when you access the DataTable directly with the cells() method.

Transformers are applied when you use for instance:

dataTable.asLists[String](classOf[String]) // java.util.List[java.util.List[String]]
// Or
dataTable.asScalaRawLists[String] // Seq[Seq[String]]

But I think transformation is applied while creating the DataTable instance from the feature file. So when we get the instance of DataTable I expect the [empty] cells to have been replaced with empty("") string already. I'm accessing cells() of expected as a workaround to replace [empty] cells manually with a helper method replaceWithEmptyString in the validateResult to move forward for now.

mpkorstanje commented 3 years ago

The transformers are only applied when the data table is converted into something else. The idea being that you use a list of transformed objects rather then a data table.

  Then("""^The search result should match following expected value$""") { 
      (cancelationPolicyResults: Seq[CancelationPolicyResult]) => ...

  }

Or if you use it on a list of strings, that you use a list of strings instead.

  Then("""^The search result should match following expected value$""") { 
      (cancelationPolicyResults: Seq[Seq[String]]) => ...

  }
mpkorstanje commented 3 years ago

It would make sense for all .asX methods to invoke the transformer though, currently the difference between .asList(String.class) and .asList() is somewhat unexpected. We can do this on the next major version of Cucumber JVM.

manish7-thakur commented 3 years ago

The transformers are only applied when the data table is converted into something else. The idea being that you use a list of transformed objects rather then a data table.

  Then("""^The search result should match following expected value$""") { 
      (cancelationPolicyResults: Seq[CancelationPolicyResult]) => ...

  }

Or if you use it on a list of strings, that you use a list of strings instead.

  Then("""^The search result should match following expected value$""") { 
      (cancelationPolicyResults: Seq[Seq[String]]) => ...

  }

@mpkorstanje @gaeljw Thanks for clarifying this point. I was under the impression that transformers are applied while creating the DataTable instance where it's just used while converting DataTable to something else.