Updated CLI integration test to check for specific size instead of multiple of the share size

[nicoelzer]

Description

Update CLI integration test to require specific size instead of multiple of the share size

closes: #245

[codecov-commenter]

Codecov Report

Merging #429 (ddda3a8) into master (35a71e1) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #429   +/-   ##
=======================================
  Coverage   25.05%   25.05%           
=======================================
  Files          14       14           
  Lines        1864     1864           
=======================================
  Hits          467      467           
  Misses       1358     1358           
  Partials       39       39           

---

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 745bd99...ddda3a8. Read the comment docs.

[evan-forbes]

Thanks for digging into this @nicoelzer ! pls see comment and let me know if you have any further questions.

its also worth noting that the way we are adding the default padding for messages is sort of in flux, as we are removing it in #419, and then will be adding a more proper implementation in #431 #432

[evan-forbes]

while this specific message is the size of the consts.ShareSize, not all messages will be this size. The only reason that it is this size is because it gets padded. Ideally, we would have a function that calculated the padded length of the message being used in the test, and then use that to compare with the result here. I think it would also be okay to simply add a field in the test case to communicate the expected size.

[nicoelzer]

Hey @evan-forbes, thanks for the review, appreciate it!

I added the padMessage function, which adds the padding to the provided message and uses the result for comparison on the test.

[evan-forbes]

hey @nicoelzer, sorry for all the delays, we've been super busy with mamaki! in this rush, we since changed this in master

celestia-app/x/payment/client/testutil/integration_test.go

Lines 116 to 119 in 1c357cb

	msgSize, err := strconv.ParseInt(e.GetAttributes()[1].GetValue(), 10, 64)
	require.NoError(err)
	require.Equal(len(msg), int(msgSize))
	}

as part of a solution to some bugs in #419. This PR was still very helpful in getting us to think about #431 #432, and I hate to toss perfectly good work, but this change isn't needed any longer since our implementation changed. Again, super sorry that this contribution isn't being recognized fully by accepting this PR, it was made precisely at the time where we were already in the process of changing it. 🙁

it is good work though 🙂

[nicoelzer]

Hey @evan-forbes,

no worries at all and thank you for the feedback. I will pick up some other open issues and looking forward to further contribute! And of course, congrats on the launch of the testnet!!