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

Remove UTF8 test #1346

Merged
merged 3 commits into from Oct 10, 2018

Conversation

Projects
4 participants
@jmjatlanta

jmjatlanta commented Sep 28, 2018

Fixes #1326

Test was verifying isalpha function with classic and normal locales, which failed if UTF8 was not available. Such a test of a library function is not useful, and has been removed.

Tested with Ubuntu 18.04 (with UTF8 available), chain_test ran without errors.

I would feel better if someone without UTF8 would compile and run the chain_test, just to make sure such a problem does not exist in another test. @abitmore you mentioned it was not working for you. Could you give this PR a try?

@jmjatlanta jmjatlanta requested a review from abitmore Sep 28, 2018

@oxarbitrage

This comment has been minimized.

Member

oxarbitrage commented Sep 28, 2018

i think i added this code to the tests in the context of https://github.com/bitshares/bsips/blob/master/bsip-0037.md but trying to remember why.

@oxarbitrage

This comment has been minimized.

Member

oxarbitrage commented Sep 28, 2018

trying to find out more. the commit that introduced the locales into the test was 5919156

in the context of #705

@oxarbitrage

This comment has been minimized.

Member

oxarbitrage commented Oct 1, 2018

@jmjatlanta i missed this comment #1326 (comment)

can you create the test case @pmconrad mentions ? i want to make sure assets can be created on windows/macos with the allowed characters.

@pmconrad

This comment has been minimized.

pmconrad commented Oct 1, 2018

The test I'd like to see should make sure that they cannot be created with non-ascii names. We already have tests with allowed characters.

@oxarbitrage

This comment has been minimized.

Member

oxarbitrage commented Oct 1, 2018

The test I'd like to see should make sure that they cannot be created with non-ascii names. We already have tests with allowed characters.

yes, exactly, thank you.

@jmjatlanta

This comment has been minimized.

jmjatlanta commented Oct 1, 2018

I would be happy to modify the test to assure that non-ascii characters cannot be used in an asset name. How strict should it be? I would assume that ascii-but-not-printable is not allowed. What about space? quote? double quote?

If this is documented somewhere, please point me to it. I will look as well.

@pmconrad

This comment has been minimized.

pmconrad commented Oct 1, 2018

Apparently protocol/account.cpp::is_valid_name explicitly lists allowed characters, no need for additional test case there IMO.
protocol/asset_ops.cpp uses isalpha, isupper, isdigit but should be restricted to ASCII chars.

@jmjatlanta

This comment has been minimized.

jmjatlanta commented Oct 1, 2018

The modified tests checks (non-extended, 0 through 127) ASCII, and it appears that 0-9 are valid in the middle and end of asset names, and A-Z are valid anywhere in asset names. Should anything else be tested?

@pmconrad

This comment has been minimized.

pmconrad commented Oct 1, 2018

What we want to test here is this:

  • Find some char values that are satisfy isalpha, isupper and/or isdigit in a different locale, set default locale to that and verify that assets can't be created.

(Setting the default locale reopens the original problem. Better ideas welcome.)

@abitmore abitmore added this to In progress in Feature release (201810) via automation Oct 1, 2018

@abitmore abitmore added this to the 201810 - Feature Release milestone Oct 1, 2018

@abitmore

Please wrap long lines.

@@ -519,6 +573,16 @@ BOOST_AUTO_TEST_CASE( asset_name_test )
sign( tx_hf620, alice_private_key );
PUSH_TX( db, tx_hf620 );
// assets with invalid characters should not be allowed
for (int c = 0; c < 128; ++c)

This comment has been minimized.

@abitmore

abitmore Oct 1, 2018

Member

How about chars in the 128-255 range? See example here: https://en.cppreference.com/w/cpp/string/byte/isalpha

@@ -519,6 +573,16 @@ BOOST_AUTO_TEST_CASE( asset_name_test )
sign( tx_hf620, alice_private_key );
PUSH_TX( db, tx_hf620 );
// assets with invalid characters should not be allowed

This comment has been minimized.

@abitmore

abitmore Oct 1, 2018

Member

Putting tests here means only checks after the hard fork. Please make sure it's appropriate.

@jmjatlanta

This comment has been minimized.

jmjatlanta commented Oct 1, 2018

isalpha can return a locale-ized result. So, it is totally possible to get isalpha to return true on something outside the standard ascii ranges. But to put that in a test means we will be making the test locale-specific. So yes, no easy answer for testing.

The reverse may be more possible. Stop using methods like isalpha and instead check for ranges.

Or allow unicode characters and their accompanying warts (warts to the English folks, that is) as asset names.

@abitmore

This comment has been minimized.

Member

abitmore commented Oct 1, 2018

It's clear that we only accept ASCII characters as asset symbols. I'd say don't waste time on this, no need to test only for test.

@jmjatlanta

This comment has been minimized.

jmjatlanta commented Oct 1, 2018

It's clear that we only accept ASCII characters as asset symbols. I'd say don't waste time on this, no need to test only for test.

If someone with a foreign locale wants to create an asset using their codepage, will it fail as it should?

That is what I would like to test for. But I am struggling to find a way to test it in a way that will not cause the test to fail when that codepage is not available.

If you are sure that the code fails as it should when it should, I'll be happy to put this to bed.

@pmconrad

This comment has been minimized.

pmconrad commented Oct 2, 2018

We have changed all (I hope) locale-dependent calls to explicitly specify classic locale. Please review relevant code, and if you think it's ok we can merge this PR without adding new tests.

@jmjatlanta

This comment has been minimized.

jmjatlanta commented Oct 3, 2018

I have verified that currently, all calls to isalpha include the classic locale. All calls to isdigit related to the creation of a new asset also use the classic locale. There are a few places where isdigit is used without specifying the locale, but not in the code that creates a new asset.

So, are we worried that someone will change some code that will provide a loophole for asset names? If so, this test could be a defense, albeit imperfect. If we are not worried about changes in the code, I can remove the piece of the test I added.

@pmconrad

This comment has been minimized.

pmconrad commented Oct 3, 2018

I hate to delete test cases, so I tend to say keep them in (but please add chars 128-255 like abit suggested).

Feature release (201810) automation moved this from In progress to In Testing Oct 10, 2018

@pmconrad

Thanks!

@pmconrad pmconrad merged commit 0e15a05 into develop Oct 10, 2018

3 checks passed

ci/dockercloud Your tests passed in Docker Cloud
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

Feature release (201810) automation moved this from In Testing to Done Oct 10, 2018

@pmconrad pmconrad deleted the jmj_1326 branch Oct 10, 2018

@pmconrad pmconrad referenced this pull request Oct 10, 2018

Closed

Test case failed on chain_test #1326

1 of 17 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment