Skip to content

Conversation

st0012
Copy link
Member

@st0012 st0012 commented Apr 2, 2022

  1. Add rake test_console for just running tests in test/console/*_test.rb and test/*_test.rb.
  2. Use rake test_console in the ruby workflow. The end result should be the same as it now runs rake test, which skips all tests under test/protocol/ by default.
  3. Since protocol tests can now be skipped via different test tasks, the PROTOCOL_TEST is no more needed.
  4. rake test can now run all tests without flipping PROTOCOL_TEST on and off.

Testing tasks after the change:

  • rake test_console all console + test framework related (e.g. assertion helpers) tests.
  • rake test_protocol all protocol related tests.
  • rake test all of the above.

@st0012
Copy link
Member Author

st0012 commented Apr 2, 2022

Test failures are caused by ruby/net-http#49

@st0012 st0012 force-pushed the improve-test-tasks branch 3 times, most recently from 383d64c to a54fae0 Compare April 4, 2022 20:43
@st0012
Copy link
Member Author

st0012 commented Apr 4, 2022

@ono-max can you also take a look at this? thx

@ono-max
Copy link
Member

ono-max commented Apr 5, 2022

Could you add desc method to each task so that users can confirm their behavior?

@ko1
Copy link
Collaborator

ko1 commented Apr 5, 2022

I can accept rake test_debug and rake test_protocol.
I can accept rake test_all (for example) for all tests.

But I can not accept rake test default behavior for both. We disabled protocol tests on default because it is not stable now, so we can change default rake test when we makes it stable.

BTW, do we have another unit tests like test/*_test.rb?

@st0012
Copy link
Member Author

st0012 commented Apr 5, 2022

@ko1 We have test/*_test.rb for testing clients, coloring, console tests' assertion helpers...etc. So I think it's reasonable to test them with test_debug.

@st0012 st0012 force-pushed the improve-test-tasks branch 2 times, most recently from 735ef76 to 56d3096 Compare April 5, 2022 08:39
@st0012
Copy link
Member Author

st0012 commented Apr 5, 2022

@ono-max @ko1 btw, do you think test/console/ and test_console will be better names? I feel like console vs protocol conveys the difference more clearly (interface)

@ono-max
Copy link
Member

ono-max commented Apr 5, 2022

For me, it'll be clearer. I agree with you.

@ono-max
Copy link
Member

ono-max commented Apr 6, 2022

Can we add a TODO comment about rake test?

@st0012 st0012 force-pushed the improve-test-tasks branch from 56d3096 to e25d277 Compare April 6, 2022 08:24
@ko1
Copy link
Collaborator

ko1 commented Apr 17, 2022

Does it correct?

  • test_debug: REPL tests (test/debug/*) and test/*_test.rb?
  • test_protocol: Protocol tests (test/protocol/*)
  • test_all: test_debug + test_protocol

Also

@ono-max @ko1 btw, do you think test/console/ and test_console will be better names? I feel like console vs protocol conveys the difference more clearly (interface)

means do you want to rename test/debug/ to test/console/ and rename the proposed rule test_debug to test_console'?

I'm okay for both. I like "repl" terminology more, but "console" is also acceptable.


I want to separate integrate tests (test/debug/* and test/protocol/) and unit tests (test/_test.rb). The difference between them are making other processes or not. I understand test/*_test.rb is for console features, so maybe test/console/scenario/ is a better name for current test/debug/ files?

@st0012
Copy link
Member Author

st0012 commented Apr 17, 2022

@ko1 How about:

  • test/console/scenarios/*_test.rb - for break_test.rb..etc.
  • test/console/*_test.rb - for test_utils_test.rb..etc.
  • test/protocol/requests/*_test.rb - for request-based tests.
  • test/protocol/raw/*_test.rb - for all _raw_test.rb, but they won't raw in their names anymore.

@ko1
Copy link
Collaborator

ko1 commented Apr 17, 2022

test/console/scenarios/

I don't want to introduce plural form because it should be an attribute. It doesn't mean a set of scenarios, but it should mean scenario-type tests. If it introduces unpleasant sense, "repl" seems clearer (test/console/repl/).

test/protocol/requests/*_test.rb - for request-based tests.

What is "request-based tests"?
(where the "request" from?)

@st0012
Copy link
Member Author

st0012 commented Apr 17, 2022

What is "request-based tests"?

The newer protocol tests are written in a way that focuses on individual requests. For example, BreakTest are usually focuses on the requests that sends setBreakpoints command.

req_add_breakpoint 5

That makes me think about the requests specs in Rails and that's why I call it that way.

But of course, those protocol tests usually involve a lot more requests to complete an entire test case. So calling it "request-based" may be confusing. I don't have better alternative though, perhaps scenario(s) also work for them?

It doesn't mean a set of scenarios, but it should mean scenario-type tests.

I don't understand the difference. Those tests are different scenarios.

We also usually put feature specs under test/features or spec/features instead of test/feature

@st0012 st0012 force-pushed the improve-test-tasks branch 3 times, most recently from 00bd1fd to 74a5da1 Compare April 17, 2022 17:40
@st0012
Copy link
Member Author

st0012 commented Apr 17, 2022

@ko1 I've finished the debug -> console renaming but I'd like to open another PR for the test folder restructuring. I created this PR mainly for dropping the PROTOCOL_TEST so I hope it can be merged first 😁

st0012 added 5 commits May 16, 2022 10:22
Since debug/protocol tests are now run via separate commands, we don't
need to have a flag for skipping protocol tests. That'll make things
easier to understand and reduce the noise.
@st0012 st0012 force-pushed the improve-test-tasks branch from 815c687 to 1fc2d60 Compare May 16, 2022 09:24
@ko1 ko1 merged commit a2c95ef into ruby:master May 17, 2022
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.

3 participants