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.

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

Switch tests to redpanda #89

Merged
merged 5 commits into from
Mar 24, 2023
Merged

Switch tests to redpanda #89

merged 5 commits into from
Mar 24, 2023

Conversation

bobbyiliev
Copy link
Collaborator

What type of PR is this? (check all applicable)

  • ♻️ Refactor
  • ✨ Feature
  • 🐛 Bug Fix
  • 👷 Optimization
  • 📝 Documentation Update
  • 🚩 Other

Description

Switching the Kafka + CSR + Zookeeper containers to Redpanda.
Adding a path ignore for the .md, .txt files, and examples directory.

Not 100% sure if there will be any actual benefits of switching to the image caching suggestion as we only have the Redpanda image that will be pulled and we need to make sure that the datagen image is built fresh on every run to prevent some weird results. Open to suggestions!

Related Tickets & Documents

Closes #87

Added to documentation?

  • 📜 readme
  • 🙅 no documentation needed

[optional] What gif best describes this PR or how it makes you feel?

@chuck-alt-delete
Copy link
Collaborator

Looking at the integration test results, it seems like several of these tests are actually failing. Things like

404 subject Products-value doesn’t exist

There’s also a failure on tests/schema2.sql that I think is a separate bug.

Regardless, these tests should show fail. Curious why they aren’t.

Copy link
Collaborator

@chuck-alt-delete chuck-alt-delete left a comment

Choose a reason for hiding this comment

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

Integration tests show pass, but if you look at the details, they are actually failing.

shoot, should have put all my other comments in here

@chuck-alt-delete
Copy link
Collaborator

Ah yes, here’s one thing. This error catch should probably exit 1: https://github.com/MaterializeInc/datagen/blob/main/src/kafka/cleanKafka.ts#L25

@chuck-alt-delete
Copy link
Collaborator

Another thing — looks like the avsc file with avro output test has a -p sql that it shouldn’t

@chuck-alt-delete
Copy link
Collaborator

Ah yes, here’s another place where we should exit 1:

https://github.com/MaterializeInc/datagen/blob/main/datagen.ts#L104

@bobbyiliev
Copy link
Collaborator Author

Good catch @chuck-alt-delete! I've added those exit 1 changes and fixed the mismatching prefixes in the tests!

Copy link
Collaborator

@chuck-alt-delete chuck-alt-delete left a comment

Choose a reason for hiding this comment

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

Just need to add exit 1 here: https://github.com/MaterializeInc/datagen/blob/main/datagen.ts#L104

and remove schema2.sql from the tests (that’s a bug we should look into separately — it has to do with using multiple columns as a primary key)

@bobbyiliev
Copy link
Collaborator Author

Just need to add exit 1 here: https://github.com/MaterializeInc/datagen/blob/main/datagen.ts#L104

and remove schema2.sql from the tests (that’s a bug we should look into separately — it has to do with using multiple columns as a primary key)

Had to update the Parse schema process exit here as well so that those problems would cause an error.

Also disabling the SQL to avro tests as there is a bug with the naming convention of the schemas, eg the schemas can not be registered when there is a dot in the name name:

| ✖  Failed to register schema.  ResponseError: Confluent_Schema_Registry - name doesn't match namespace {"name":"ecommerce.products","type":"record","fields":[{"name":"id","type":"int"},{"name":"name","type":"string"},{"name":"merchant_id","type":"int"},{"name":"price","type":"int"},{"name":"status","type":"boolean"},{"name":"created_at","type":"string"}],"namespace":"com.materialize"}

The error message indicates that the name of the schema, "ecommerce.products", does not match the namespace specified in the schema, "com.materialize". In the Confluent Schema Registry, the name of the schema should include the namespace, so the correct name for this schema would be "com.materialize.ecommerce.products" or we should not allow dots to be used in the schema names instead.

We should open a separate issue for this.

Copy link
Collaborator

@chuck-alt-delete chuck-alt-delete left a comment

Choose a reason for hiding this comment

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

Sweet! Nice work fixing up these tests!

@bobbyiliev bobbyiliev merged commit a1dc5cd into main Mar 24, 2023
@bobbyiliev bobbyiliev deleted the redpanda-integration-tests branch March 24, 2023 15:48
recursethis pushed a commit to recursethis/materialize-datagen that referenced this pull request Feb 26, 2024
* Switch tests to redpanda

* Fix failing tests

* Fix process exit codes

* Change schema2.sql tests

* Run npm update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve integration tests
2 participants