Skip to content

Conversation

@Shakadak
Copy link
Member

@Shakadak Shakadak commented Feb 5, 2024

I think there is a more fundamental improvement that can be done to fix this issue by reviewing how we use Stream.resource, but this is the smallest change we can do to improve the situation right now

cf #97

Comment on lines +1 to +4
defmodule Issue97Test do
use ExUnit.Case

test "stream_tags doesn't hang when used with stream take on empty xml" do
Copy link
Contributor

@half-shell half-shell Feb 5, 2024

Choose a reason for hiding this comment

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

Suggested change
defmodule Issue97Test do
use ExUnit.Case
test "stream_tags doesn't hang when used with stream take on empty xml" do
defmodule SweetXmlTest do
use ExUnit.Case
describe "stream_tags/3" do
# c.f. https://github.com/kbrw/sweet_xml/issues/97
test "should not hang when used with Stream.take/2 on an empty xml" do

💅
While I do like the fact that this helps linking these tests to the issue raised to gain an understanding of the reason it was added, i think having a meaningful test module name might be better for overall comprehension and reading of test files.
Especially considering there are several ways of achieving the same results through other means; for instance, the commit itself already have that information. We could even put a link to the Github issue in the commit description, and / or even in a @doc or @docmodule attribute here.

@half-shell half-shell added the bug label Feb 5, 2024
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.

2 participants