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

Setup CI for adapter with service dependencies #8

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

timkpaine
Copy link
Member

No description provided.

@timkpaine timkpaine force-pushed the tkp/adapters branch 3 times, most recently from 7b48be6 to 0f91511 Compare February 4, 2024 13:29
@timkpaine timkpaine added the tag: internal Issues and PRs for maintainance of the project - not interesting to external users label Feb 6, 2024
@timkpaine timkpaine force-pushed the tkp/adapters branch 7 times, most recently from 93e5d1b to 8693a5a Compare February 19, 2024 17:01
@timkpaine timkpaine force-pushed the tkp/adapters branch 2 times, most recently from 87d1023 to 95b9a53 Compare February 28, 2024 14:57
@timkpaine timkpaine closed this Mar 13, 2024
@timkpaine timkpaine deleted the tkp/adapters branch March 13, 2024 23:17
@timkpaine timkpaine restored the tkp/adapters branch May 3, 2024 14:01
@timkpaine timkpaine reopened this May 3, 2024
@timkpaine timkpaine force-pushed the tkp/adapters branch 2 times, most recently from 08995d1 to 7a07db5 Compare May 24, 2024 08:30
@timkpaine timkpaine marked this pull request as ready for review June 30, 2024 03:29
@timkpaine timkpaine added the adapter: kafka Issues and PRs related to the Apache Kafka adapter label Jun 30, 2024
@timkpaine
Copy link
Member Author

ok this is finally working ok, does not appreciably increase CI/CD time and worth it until we can separate adapters.

@timkpaine timkpaine force-pushed the tkp/adapters branch 2 times, most recently from 053e0e8 to 6909355 Compare July 29, 2024 18:02
Copy link
Collaborator

@ptomecek ptomecek left a comment

Choose a reason for hiding this comment

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

There are two warnings from pytest that come up:

csp/tests/adapters/test_kafka.py:297
/home/runner/work/csp/csp/csp/tests/adapters/test_kafka.py:297: PytestRemovedIn9Warning: Marks applied to fixtures have no effect
See docs: https://docs.pytest.org/en/stable/deprecations.html#applying-a-mark-to-a-fixture-function
@pytest.mark.skipif(not os.environ.get("CSP_TEST_KAFKA"), reason="Skipping kafka adapter tests")

and

csp/tests/adapters/test_kafka.py:373
/home/runner/work/csp/csp/csp/tests/adapters/test_kafka.py:373: PytestRemovedIn9Warning: Marks applied to fixtures have no effect
See docs: https://docs.pytest.org/en/stable/deprecations.html#applying-a-mark-to-a-fixture-function
@pytest.mark.skipif(not os.environ.get("CSP_TEST_KAFKA"), reason="Skipping kafka adapter tests")

I'm not sure that it is intended for test_raw_pubsub and test_invalid_topic to be tagged with @pytest.fixture. These two tests don't appear in the list of tests that were run via the action either.

Makefile Outdated Show resolved Hide resolved
csp/adapters/kafka.py Show resolved Hide resolved
@timkpaine
Copy link
Member Author

There is a race condition in one of the tests it seems, wanted to note here in case it comes up. I cannot fully hammer it down, and unclear if its an actual issue with the historical->realtime switch or if the test itself has buggy assumptions.

@robambalu
Copy link
Collaborator

There is a race condition in one of the tests it seems, wanted to note here in case it comes up. I cannot fully hammer it down, and unclear if its an actual issue with the historical->realtime switch or if the test itself has buggy assumptions.

do you have an example link

@timkpaine
Copy link
Member Author

@robambalu
Copy link
Collaborator

https://github.com/Point72/csp/actions/runs/10149403062/job/28067369034

Based on the test and the error message it looks like whats happening is that the sub is missing the first pub. Its possible the daemon is late to register the subscriber before the first publish. One way to try and fix it is to delay the first pub a bit. Another possibility is to compare the last N msgs rather than the first N

@timkpaine timkpaine force-pushed the tkp/adapters branch 2 times, most recently from fc66f1e to 8bca913 Compare September 2, 2024 00:04
@timkpaine timkpaine force-pushed the tkp/adapters branch 2 times, most recently from c64cb93 to 21229ed Compare September 10, 2024 14:11
Signed-off-by: Tim Paine <3105306+timkpaine@users.noreply.github.com>
Signed-off-by: Tim Paine <3105306+timkpaine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adapter: kafka Issues and PRs related to the Apache Kafka adapter tag: internal Issues and PRs for maintainance of the project - not interesting to external users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants