Skip to content

Conversation

@zcsizmadia
Copy link
Contributor

Make sure you have checked all steps below.

Jira

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

  • My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain Javadoc that explain what it does

@github-actions github-actions bot added the C# label Mar 10, 2022
@zcsizmadia
Copy link
Contributor Author

zcsizmadia commented Mar 10, 2022

@martin-g @RyanSkraba We have some work in progress and PR done for avrogen. This PR makes very minimal (non-functional) changes to avrogen, however it adds a good amount of testing for the code generation. IMO this could be a very solid base line testing before we make any changes to avrogen. This changes should mofify or add new unit tests. This way we have more confidence in the changes. I had a pending PR #1574 which had these unit tests, however I think it is a better sequencing if we add the baseline testing first to reflect and test the current avrogen status.

The tests include the following:

  1. Command line sanity checks /w return codes
  2. Compiling avro schemas into cs files AND compiling them with the runtime Roslyn compiler (DOM cannot be used in dotnet core). That old commented out code is revived by using the roslyn compiler,
  3. Checking the generated cs files directory hierarchy
  4. Checking the types included in the dynamically compiled assembly
  5. Instantiate object for the the types included in the dynamically compiled assembly
  6. Because of this I added a unit test GenerateSchemaWithNamespaceMapping_Bug_AVRO_2883 which validates that the https://issues.apache.org/jira/browse/AVRO-2883 bug is valid, since the compilation will fail. As soon as it is fixed with AVRO-3425: Extend C# avrogen namespace mapping to models and members as well #1574 that unit test can be removed since it will fail, becuase the issue is fixed
  7. Namespace mapping for single objects, where the above bug is not a concern
  8. Namespace mapping with reserved keywords in the namespace

If you agree we can prioritize this PR so, e.g. #1578 can add unit tests to validate the new skipped directory structure

@zcsizmadia
Copy link
Contributor Author

The Travis CI support is very slooow. Is there anything that travis.yml covers, which is not covered by the git flow actions? Can it be retired? Many times Travis takes hours to be finished.

@KyleSchoonover
Copy link
Contributor

I'm good with the intention of this, but I do want to break the functionality of the AvroGen class into a separate class. Keep the "program" entry point to just deal with parsing the input. The other option is taking advantage of the strong naming to sign the assembly.

@zcsizmadia
Copy link
Contributor Author

Note that there is an AvroGenTests, for that reason. Most functionality will be moved to the libraryaa you said. And AvroGenToolTests will be the tool specific tests. This will be done in steps to make sure that current functionality is not altered by the actual rework.

@RyanSkraba RyanSkraba merged commit 00be26a into apache:master Mar 18, 2022
@RyanSkraba
Copy link
Contributor

Should this be cherry-picked to 1.11.1 ? It seems like a good candidate!

RyanSkraba pushed a commit that referenced this pull request Mar 18, 2022
* Add avrogen unit tests

* Hide dynamic compiler errors

* Add asserts

* Bump MSBUild package versions

* Tweak dotnet 6.0 install

* Add comments

* Use 6.0.2xx channel

* Revert dotnet-install change

* Add codegen tests

* Add test for contextual keyword in namespace

* Add AvroGenHelper

* Update lang/csharp/src/apache/main/CodeGen/CodeGenUtil.cs

Co-authored-by: Martin Grigorov <[email protected]>

* Update lang/csharp/src/apache/test/AvroGen/AvroGenHelper.cs

Co-authored-by: Martin Grigorov <[email protected]>

Co-authored-by: Zoltan Csizmadia <[email protected]>
Co-authored-by: Martin Grigorov <[email protected]>
@zcsizmadia
Copy link
Contributor Author

@RyanSkraba adding it to 1.11.1 makes sense. These unit tests IMO are a must have to provide a base line test coverage to the code generation and avrogen, which was not covered before.

@zcsizmadia zcsizmadia deleted the avro-3446-add-avrogen-unit-tests branch April 19, 2022 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants