Replace Boost-based Natspec test case with one derived from `SyntaxTest` #14506

cameel · 2023-08-21T11:13:39Z

Fixes #14433.
Depends on #14505.

I recommend reviewing commits individually in this PR. There's a lot of file moves and simple changes that can be easily skimmed, but this is not apparent in the unified diff.

nikola-matic · 2023-08-21T11:18:14Z

Oh wow, how long did this take to move from cpp to isoltest? 🚀

cameel · 2023-08-21T11:26:01Z

test/libsolidity/natspecJSON/return_param_amount_differs.sol

This one and return_param_amount_differs.sol were originally in syntaxTests/natspec/invalid/, even though they work just fine. The number of return parameters does not differ like the name implies and perhaps it should. Need to investigate.

cameel · 2023-08-21T11:36:25Z

Oh wow, how long did this take to move from cpp to isoltest? 🚀

If you mean moving the test cases, not much. I spent a few hours writing a test parser. Later I ran it and it did all the work for me (see #14433 (comment)). That was actually the easiest part :)

I spent more trying to actually reuse isoltest base classes. I went through a few dead ends in the process. Still, it wasn't by itself that hard and I was actually sitting on this being 90% finished, and waiting for my other refactors to go in.

Then it turned out that this last 10% is more annyoing than expected and some features I needed for one more complex test case could not be easily reused. So before the weekend I rewrote the test case on top of SyntaxTest (see #14505) and got this.

nikola-matic

I'll go over the SolidityNatspecJSON.cpp to double check whether you've moved all of the tests, but that will take some time unfortunately.

nikola-matic · 2023-08-21T11:30:09Z

test/libsolidity/NatspecJSONTest.cpp

+		));
+
+	startPosition += 2;
+	if (startPosition < _line.size() && _line[startPosition] == ' ')


Should this behave as a trim function, or do we always expect a maximum of one whitespace after //?

No, I intentionally only strip // . If there's more indentation there, it's a part of the value. In many cases it won't matter, but in some it might. I'm considering moving this helper to some more general location later, so better not to assume that. Actually, even with JSON it might matter if the parser allows multi-line strings (which I think jsoncpp does). And if you want to print the extracted value, it may look weird without whitespace.

test/libsolidity/NatspecJSONTest.cpp

nikola-matic · 2023-08-21T11:46:27Z

test/libsolidity/natspecJSON/custom.sol

+// ----
+// ----


Why are there two of these?

That's the feature I added in #14505. CommonSyntaxTest now supports extra expectations after the error list and these are separated with an extra // ----. The base class stops reading from the stream there and the derived class can continue, interpreting the content any way it wants.

NatspecJSONTest interprets it as Natspec JSON. It does it inside parseCustomExpectations(), which is virtual and provided specifically so that the derived class can put such a logic there. We it could do this already in constructor, like CommonSyntaxTest does, though I don't really like that mechanism - it's too implicit and I feel like it would be too easy to mess up the order.

I could have used any other separator, but reusing this one seemed logical. The only downside is that we have two when the error section is empty. On the other hand, I don't have to worry about inventing a new one and making sure it's not used for something else.

cameel · 2023-08-21T12:24:34Z

test/libsolidity/natspecJSON/custom_inheritance.sol

+//     "kind": "user",
+//     "methods": {},
+//     "version": 1


By the way, we could filter out kind and version to make the expectations a little shorter. Should we? version is the same for all tests and kind is redundant.

The original tests were inconsistent about it. Some had these fields, but many did not, because the Boost test case allowed omitting them. The downside was that they were not checked, even if included - I actually find one or two places where we were expecting the wrong value. Checking them does not add all that much value though.

I'd keep them.

matheusaaguiar

I manually checked that all tests from SolidityNatspecJSON.cpp were transferred.

test/libsolidity/natspecJSON/dev_documenting_no_paramname.sol

nikola-matic

So, the main portion of the review I've done locally - i.e. playing around with the tests to see whether it's detecting failures properly, and indeed it does.

The comments I left are really more out of curiosity than anything else. I think it's perfectly fine to merge this as soon as the osx jobs are fixed, i.e. no point in waiting much further.

nikola-matic · 2023-09-07T19:00:45Z

test/libsolidity/NatspecJSONTest.cpp

+	case NatspecJSONKind::Devdoc: _output << "devdoc"; break;
+	case NatspecJSONKind::Userdoc: _output << "userdoc"; break;


You don't indent cases inside a switch statement? Is that our style?

Yes. I actually don't like that style too much (I'd prefer the cases to be indented one level), but I stick to it because that's the style used around the code base.

nikola-matic · 2023-09-07T20:43:20Z

test/libsolidity/NatspecJSONTest.cpp

+	// sometimes uint and they compare as different even when both are 1.
+	return
+		SyntaxTest::expectationsMatch() &&
+		prettyPrinted(obtainedNatspec()) == prettyPrinted(m_expectedNatspecJSON);


obtainedNatspec() returns both devdoc and userdoc for requested contracts, and many tests only expect either userdoc or devdoc; how does this comparison work?

many tests only expect either userdoc or devdoc

It used to be the case in the boost tests but if you look at the new expectations, they now always have both userdoc and devdoc.

See commit e702640 (natspecJSON: Generate missing expectations (including errors)), which adds those expectations (auto-generated by isoltest).

nikola-matic · 2023-09-07T20:47:54Z

test/libsolidity/NatspecJSONTest.cpp

+				output += "\n\n";
+			first = false;
+
+			output += contractName + " " + toString(jsonKind) + "\n";


Multisource tests also include the file name, e.g. Testfile.sol:Token devdoc, so where does said filename and : delimiter come from? Is it inherited from SyntaxTest?

That's the fully-qualified contract name. It comes from compiler().contractNames() and it's what all of its functions expect when the parameter is a contract name. That's how CompilerStack refers to contracts.

…nversion - Expectation order matching the order of contracts in the source - Typos in test names - Redundant prefixes in test names - Wrong 'king' in some expectations (it's not checked by the test suite)

…ot be handled correctly by the script - dev_multiple_params_mixed_whitespace has whitespace that is not completely preserved - dev_explicit_inherit_complex is a multi-file test

…/*.sol tests

…e missing

cameel added the testing 🔨 label Aug 21, 2023

cameel requested review from ekpyron and matheusaaguiar August 21, 2023 11:13

cameel self-assigned this Aug 21, 2023

cameel changed the title ~~Replace Boost-based Natspec test case with one derived~~ Replace Boost-based Natspec test case with one derived from SyntaxTest Aug 21, 2023

cameel added the has dependencies The PR depends on other PRs that must be merged first label Aug 21, 2023

cameel requested review from nikola-matic and removed request for ekpyron August 21, 2023 11:16

cameel force-pushed the extracted-natspec-json-tests branch from 5e232ea to 3bdc92f Compare August 21, 2023 11:22

cameel commented Aug 21, 2023

View reviewed changes

nikola-matic reviewed Aug 21, 2023

View reviewed changes

cameel commented Aug 21, 2023

View reviewed changes

cameel force-pushed the syntax-test-more-extension-points branch from 80511e8 to f4146ee Compare August 21, 2023 15:08

cameel force-pushed the extracted-natspec-json-tests branch 2 times, most recently from a8cd268 to a6e703b Compare August 22, 2023 13:45

cameel force-pushed the syntax-test-more-extension-points branch from 5fa0993 to bf0d51d Compare August 22, 2023 13:45

cameel force-pushed the extracted-natspec-json-tests branch from a6e703b to 7fc81d0 Compare August 22, 2023 13:46

cameel force-pushed the syntax-test-more-extension-points branch from bf0d51d to 7768f26 Compare August 22, 2023 19:17

cameel force-pushed the extracted-natspec-json-tests branch from 7fc81d0 to ae25418 Compare August 22, 2023 19:19

cameel force-pushed the syntax-test-more-extension-points branch from 7768f26 to e847596 Compare August 23, 2023 16:00

Base automatically changed from syntax-test-more-extension-points to develop August 23, 2023 16:48

cameel force-pushed the extracted-natspec-json-tests branch from ae25418 to f29ac54 Compare August 23, 2023 16:51

cameel marked this pull request as ready for review August 23, 2023 16:52

matheusaaguiar previously approved these changes Aug 23, 2023

View reviewed changes

test/libsolidity/natspecJSON/dev_documenting_no_paramname.sol Outdated Show resolved Hide resolved

cameel dismissed matheusaaguiar’s stale review via 2674048 August 25, 2023 17:16

cameel force-pushed the extracted-natspec-json-tests branch from f29ac54 to 2674048 Compare August 25, 2023 17:16

This was referenced Aug 25, 2023

Generalize helpers for indenting and prefixing #14517

Merged

AnalysisFramework refactor #14518

Merged

cameel force-pushed the extracted-natspec-json-tests branch from 2674048 to 9ed96d4 Compare September 7, 2023 12:37

cameel mentioned this pull request Sep 7, 2023

File-level events #14550

Merged

nikola-matic approved these changes Sep 8, 2023

View reviewed changes

nikola-matic removed the has dependencies The PR depends on other PRs that must be merged first label Sep 8, 2023

cameel added 8 commits September 11, 2023 18:06

NatspecJSONTest based on SyntaxTest

91cc72b

SolidityNatspecJSON: A few tweaks and small fixes before automatic co…

1041f07

…nversion - Expectation order matching the order of contracts in the source - Typos in test names - Redundant prefixes in test names - Wrong 'king' in some expectations (it's not checked by the test suite)

SolidityNatspecJSON: Manual conversion of two test cases that would n…

ba019e5

…ot be handled correctly by the script - dev_multiple_params_mixed_whitespace has whitespace that is not completely preserved - dev_explicit_inherit_complex is a multi-file test

SolidityNatspecJSON: Convert Boost-based test cases into natspectJSON…

99bfdf9

…/*.sol tests

natspecJSON: Include version and kind fields in expectations wher…

481c725

…e missing

natspecJSON: Generate missing expectations (including errors)

d083925

Move Natspec syntax tests to natspecJSON

dc68480

Generate expectations for moved Natspec syntax tests

b63a940

cameel force-pushed the extracted-natspec-json-tests branch from 9ed96d4 to b63a940 Compare September 11, 2023 16:06

cameel merged commit 64a0f62 into develop Sep 11, 2023
1 check passed

cameel deleted the extracted-natspec-json-tests branch September 11, 2023 17:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Boost-based Natspec test case with one derived from `SyntaxTest` #14506

Replace Boost-based Natspec test case with one derived from `SyntaxTest` #14506

cameel commented Aug 21, 2023

nikola-matic commented Aug 21, 2023

cameel Aug 21, 2023

cameel commented Aug 21, 2023 •

edited

Loading

nikola-matic left a comment

nikola-matic Aug 21, 2023

cameel Aug 21, 2023

nikola-matic Aug 21, 2023

cameel Aug 21, 2023

cameel Aug 21, 2023 •

edited

Loading

nikola-matic Aug 21, 2023

matheusaaguiar left a comment

nikola-matic left a comment

nikola-matic Sep 7, 2023

cameel Sep 8, 2023

nikola-matic Sep 7, 2023

cameel Sep 8, 2023

nikola-matic Sep 7, 2023

cameel Sep 8, 2023 •

edited

Loading

		case NatspecJSONKind::Devdoc: _output << "devdoc"; break;
		case NatspecJSONKind::Userdoc: _output << "userdoc"; break;

Replace Boost-based Natspec test case with one derived from SyntaxTest #14506

Replace Boost-based Natspec test case with one derived from SyntaxTest #14506

Conversation

cameel commented Aug 21, 2023

nikola-matic commented Aug 21, 2023

Choose a reason for hiding this comment

cameel commented Aug 21, 2023 • edited Loading

nikola-matic left a 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

cameel Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matheusaaguiar left a comment

Choose a reason for hiding this comment

nikola-matic left a 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

cameel Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

Replace Boost-based Natspec test case with one derived from `SyntaxTest` #14506

Replace Boost-based Natspec test case with one derived from `SyntaxTest` #14506

cameel commented Aug 21, 2023 •

edited

Loading

cameel Aug 21, 2023 •

edited

Loading

cameel Sep 8, 2023 •

edited

Loading