MirrorTest #951

jaadams5 · 2022-08-05T16:22:37Z

Test from #942 tweaked to work without any of the code changes from 942.

Need a test that writes something to the GCLI but is not written to the mirror file.

Need to explore the assertion that adding > will trigger colorization and how that relates to something not being mirrored.

As copied.

jaadams5 · 2022-08-05T16:23:13Z

@libraryaddict

Comments?

libraryaddict · 2022-08-05T16:26:52Z

On phone and will have to look at it tomorrow, but initial thought is that it does not contain a super minor change that switches it from contains text, to assert equals.

The original contains check was due to chat buffer being the original change which really didn't play well with multiple distros. Could also be the case here, not sure.

codecov · 2022-08-05T16:30:31Z

Codecov Report

Merging #951 (766b1bc) into main (f403ac0) will increase coverage by 0.04%.
The diff coverage is n/a.

❗ Current head 766b1bc differs from pull request most recent head d8c4c28. Consider uploading reports for the commit d8c4c28 to get more accurate results

@@             Coverage Diff              @@
##               main     #951      +/-   ##
============================================
+ Coverage     27.23%   27.28%   +0.04%     
- Complexity    12833    12860      +27     
============================================
  Files          1029     1029              
  Lines        160478   160478              
  Branches      35329    35329              
============================================
+ Hits          43708    43783      +75     
+ Misses       108776   108695      -81     
- Partials       7994     8000       +6

Impacted Files	Coverage Δ
...et/sourceforge/kolmafia/textui/RuntimeLibrary.java	`34.84% <0.00%> (+0.08%)`	⬆️
src/net/sourceforge/kolmafia/RequestLogger.java	`20.04% <0.00%> (+2.20%)`	⬆️
.../net/sourceforge/kolmafia/utilities/LogStream.java	`20.00% <0.00%> (+4.44%)`	⬆️
...ourceforge/kolmafia/persistence/SkillDatabase.java	`35.62% <0.00%> (+5.69%)`	⬆️
.../kolmafia/textui/command/AshSingleLineCommand.java	`100.00% <0.00%> (+10.00%)`	⬆️
...orge/kolmafia/textui/command/MirrorLogCommand.java	`62.50% <0.00%> (+43.75%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f403ac0...d8c4c28. Read the comment docs.

gausie · 2022-08-09T09:10:55Z

test/net/sourceforge/kolmafia/textui/command/MirrorCommandTest.java

+
+  @Test
+  public void testMirrorListSkills() {
+    try (var cleanups = new Cleanups(withSkill(1), withSkill(2))) {


This doesn't work on all OS I think? @midgleyc couldn't you not do this

couldn't I not do what?

This is fine, it just makes the IDE complain that cleanups is unused (which it is, but you need to assign to a variable in a try-with-resources), hence why I normally go for var cleanups = ...; try (cleanups).

IntelliJ isn't flagging var as unused for me.

does it flag any variables as unused? If it does there might be a setting I'm missing.

Yes. I normally use Analyze->Inspect. I introduced an unused variable and it was recognized as such (by being gray in the IDE text display) and flagged by the inspection.

So I did a recompile from within the IDE and the complier complained about unused. The manually run analysis did not. So I am seeing what you did

Ah it was an IntelliJ thing. So it sounds like you need to change this frono?

test/net/sourceforge/kolmafia/textui/command/MirrorCommandTest.java

Tweaked some of the test assertions to have more useful error messages if they fail. But left some others, at least until someone objects.

Another working test.

Some cleanup regarding expectation of HTML

jaadams5 · 2022-08-10T16:38:26Z

I am somewhat comfortable with these tests. I would love a failing test that showed something written to the gCLI but not the mirror but it may be that the unmodified mirror command really does mirror everything so no such test is possible.

The tests assume two things. First, a mirror file will have a .txt extension regardless of what the user typed. Second, if HTML is written to the gCLI HTML will be written to the mirror.

I'll let this sit for a day but if there is no advice to the contrary I am going to undraft it and commit it. After that we can revisit #942 and decide what should happen.

@gausie @libraryaddict

gausie

Seems good to me!

Rewrite to avoid unused variable warning.

libraryaddict · 2022-08-12T18:18:10Z

I have a review pending, asking about a test that is labeled incorrectly and made to deliberately successful using the incorrect output.

midgleyc · 2022-08-12T18:21:51Z

I think you need to submit the review before anybody except you can see it.

libraryaddict · 2022-08-10T21:03:50Z

test/net/sourceforge/kolmafia/textui/command/MirrorCommandTest.java

+    // Close the mirror
+    execute("");
+    String mirrorOutput = getMirrorLog("test_writes_html.txt");
+    assertThat(mirrorOutput, containsString("> Fake command input"));


The test contents and the method name are at odds to each other.

It's a bug that the html is not being printed as it does in gCLI, not a feature.

@jaadams5 I'm still confused about this

Specifically which test?

I broke this out because the other code and our discussions there make me think we are not talking about the same thing.

The alleged bug, to be fixed, is that a message appears in the gCLI but does not appear in the mirror. I have been unable to write a failing test for this and as a result I am inclined to believe there is no such bug.

There is a feature request that, if the text written to the gCLI contains HTML (and is rendered as such in the gCLI) then the mirror should contain the HTML as well. Tests to exercise changes to implement that feature have been preserved in this code but they have been converted to passing tests rather than commented out. It could have been clearer, I suppose, if they had been Disabled. In any event once it is established that the mirror command is mirroring everything, then the HTML inclusion can be implemented and my hope is that these tests will fail as a result and need to be changed. The tests are documenting current behavior with the expectation that changed behavior will be detected. If that is too confusing tests can be disabled.

The big thing here is the inability to find a test that demonstrates something written to the gCLI but not the mirror.

It's a bug that the html is not being printed as it does in gCLI, not a feature.

Ignoring the name, the test shows that the gCLI output and the mirror output are identical and that neither contains HTML.

Since running mafia, I could not get ">" to trigger html to the gCLI I assumed your expectation otherwise was incorrect. Maybe I was mistaken?

Ah, I see what you mean.

I believe it's a bug in that what's written to GCLI isn't exactly what's written to mirror.

Mirror writes exactly what the logger was given, but the logger applies html to the input based on rules which is what's displayed in cli.

So it's arguable if it's a bug fix or a feature request. I was talking under the assumption that it's a bug, because I expect to see contents and formatting identical to what's written in cgli in the mirror.

I believe it's a bug in that what's written to GCLI isn't exactly what's written to mirror.

We're getting closer, I think.

The original assertion was that things were being written to the gCLI but not to the mirror and in the context of the original "complaint" the possibility that the missing information was merely HTML formatting was not really considered/expected.

My thinking right now is that I want a test that doesn't use a mirror and shows HTML written to to the gCLI. That's probably in RequestLoggerTest. Once we have that then we can wrap a mirror around it and see the existing behavior as a test. I'm headed down that rabbit hole but might not come out today.

RerquestLogger lint

jaadams5 · 2022-08-12T19:16:00Z

Reverted to draft since there are a couple of things that can be resolved.

commit e512d67 Author: Samuel Gaus <sam@gaus.co.uk> Date: Fri Aug 12 22:05:12 2022 +0100 Fix up some missing path points trackers and incorrect path images (#975) * Fix some incorrect path images * Format Co-authored-by: Jamie Adams <82782908+jaadams5@users.noreply.github.com>

Figuring this out

jaadams5 · 2022-08-13T16:06:13Z

Closing in favor of #981

jaadams5 added 2 commits August 5, 2022 11:47

Initial

26da8e8

As copied.

Update MirrorCommandTest.java

2a7b5d1

jaadams5 mentioned this pull request Aug 5, 2022

Change the "mirror" command to completely mirror gCLI #942

Closed

gausie reviewed Aug 9, 2022

View reviewed changes

jaadams5 added 5 commits August 9, 2022 08:10

Merge branch 'main' into MirrorTest

25d67b4

Update MirrorCommandTest.java

b3bdbb7

Tweaked some of the test assertions to have more useful error messages if they fail. But left some others, at least until someone objects.

Update MirrorCommandTest.java

4051670

Another working test.

Merge branch 'main' into MirrorTest

80cbfef

Update MirrorCommandTest.java

eca35f7

Some cleanup regarding expectation of HTML

jaadams5 added 2 commits August 10, 2022 19:19

Merge branch 'main' into MirrorTest

ec03f4d

Merge branch 'main' into MirrorTest

e9f3827

jaadams5 marked this pull request as ready for review August 11, 2022 13:34

jaadams5 requested a review from a team as a code owner August 11, 2022 13:34

Merge branch 'main' into MirrorTest

3254f5c

gausie previously approved these changes Aug 12, 2022

View reviewed changes

Update MirrorCommandTest.java

05528f8

Rewrite to avoid unused variable warning.

jaadams5 dismissed gausie’s stale review via 05528f8 August 12, 2022 17:59

Merge branch 'main' into MirrorTest

766b1bc

libraryaddict suggested changes Aug 12, 2022

View reviewed changes

Update RequestLogger.java

362ea37

RerquestLogger lint

jaadams5 marked this pull request as draft August 12, 2022 19:15

jaadams5 added 2 commits August 12, 2022 15:21

Unused import and JavaDoc error

0f8667f

Working test with bracket

e96ad3c

jaadams5 added 5 commits August 13, 2022 09:48

Update RequestLoggerTest.java

b9e2a6e

Merge branch 'RequestLogger-tests-and-tweaks' into MirrorTest

cbe4efd

Update RequestLoggerTest.java

ad89e41

Figuring this out

d8c4c28

Figuring this out

jaadams5 closed this Aug 13, 2022

gausie deleted the MirrorTest branch September 2, 2022 14:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MirrorTest #951

MirrorTest #951

jaadams5 commented Aug 5, 2022

jaadams5 commented Aug 5, 2022

libraryaddict commented Aug 5, 2022

codecov bot commented Aug 5, 2022 •

edited

gausie Aug 9, 2022

midgleyc Aug 9, 2022

jaadams5 Aug 9, 2022

midgleyc Aug 9, 2022

jaadams5 Aug 9, 2022

jaadams5 Aug 11, 2022

gausie Aug 12, 2022

jaadams5 commented Aug 10, 2022

gausie left a comment

libraryaddict commented Aug 12, 2022

midgleyc commented Aug 12, 2022

libraryaddict Aug 10, 2022

libraryaddict Aug 11, 2022

jaadams5 Aug 12, 2022

jaadams5 Aug 12, 2022

libraryaddict Aug 12, 2022 •

edited

jaadams5 Aug 12, 2022

jaadams5 commented Aug 12, 2022

jaadams5 commented Aug 13, 2022

MirrorTest #951

MirrorTest #951

Conversation

jaadams5 commented Aug 5, 2022

jaadams5 commented Aug 5, 2022

libraryaddict commented Aug 5, 2022

codecov bot commented Aug 5, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaadams5 commented Aug 10, 2022

gausie left a comment

Choose a reason for hiding this comment

libraryaddict commented Aug 12, 2022

midgleyc commented Aug 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

libraryaddict Aug 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaadams5 commented Aug 12, 2022

jaadams5 commented Aug 13, 2022

codecov bot commented Aug 5, 2022 •

edited

libraryaddict Aug 12, 2022 •

edited