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

revert assert_batch_job helper #12

Open
soxofaan opened this issue Nov 2, 2023 · 4 comments
Open

revert assert_batch_job helper #12

soxofaan opened this issue Nov 2, 2023 · 4 comments

Comments

@soxofaan
Copy link
Member

soxofaan commented Nov 2, 2023

def assert_batch_job(job: BatchJob, assertion: bool, extra_message: str = ""):
try:
assert assertion
except AssertionError as e:

@JeroenVerstraelen you added assert_batch_job to simplify debugging integration tests I guess, but I think that this helper actually makes post-mortem investigation harder.

It's important to understand how pytest handles and report assert failures. If you use something like assert "foo" == "bar" in a test, pytest will clearly print both the values ("foo" and "bar") in the test failure report, which is extremely useful for debugging failed tests. (Note that pytest does quite a bit of magic behind the screens to make this possible)

If you replace that with a helper construct like

def assert_this(condition):
    assert condition

def test_foo()
    x = "f" + ("o" * 2)
    assert_this(x == "bar")

you lose this unique selling point of pytest as you will only see something like "assert False" in the test report, without any pointers to the actual values that were compared ("foo" and "bar" here)

I think we should revert introducing that assert_batch_job helper

@soxofaan
Copy link
Member Author

soxofaan commented Jan 18, 2024

I managed to get logging logs included in the junit report, which reduces the need for that assertion message generation of assert_batch_job (I guess it was added to automatically see the job id)

@soxofaan
Copy link
Member Author

Related: I noticed this other helper execute_batch_with_error_logging but I'm not sure if there is much added value (especially if assert_batch_job would be eliminated)

@JeroenVerstraelen
Copy link
Contributor

JeroenVerstraelen commented Mar 27, 2024

I'm a bit late in my response but I agree that we should remove assert_batch_job.

@soxofaan
Copy link
Member Author

Also note that this helper exposes internal (kibana) urls in this open source repo

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

No branches or pull requests

2 participants