Interactive syntax test tool. #3709

ekpyron · 2018-03-12T13:29:55Z

Adds an interactive test tool that runs all syntax tests and for failing tests allows to interactively edit the test or replace the test expectations with the actual test results.

Part of #3674.

Depends on #3707 and #3708.

chriseth

I think I need another run through this to understand everything :)

chriseth · 2018-03-13T16:58:45Z

test/libsolidity/FormattedPrinter.h

+		Scope(std::ostream& _stream, bool const _enabled): m_stream(_stream), m_enabled(_enabled) {}
+		~Scope() { try { if (m_enabled) m_stream << RESET; } catch(...) {} }
+		template<typename T>
+		std::ostream& operator<<(T&& t) { return m_stream << std::forward<T>(t); }


chriseth · 2018-03-13T16:59:16Z

test/libsolidity/FormattedPrinter.h

+	void enableFormatting() { m_enabled = true; }
+	void disableFormatting() { m_enabled = false; }
+
+	class Scope


Perhaps add a short ///-comment that explains the purpose of this class.

chriseth · 2018-03-13T17:00:05Z

test/libsolidity/FormattedPrinter.h

+		std::ostream& m_stream;
+		bool const m_enabled = false;
+	};
+	Scope format(std::ostream& _stream, std::vector<const char*> const& _formatting) const


char const*

A comment about how to use this function would also be in order.

chriseth · 2018-03-13T17:10:59Z

test/libsolidity/SyntaxTest.cpp

 	else
 		for (auto const& expectation: m_expectations)
-			_stream << _indent << expectation.type << ": " << expectation.message << endl;
+			format(_stream, {expectation.type == "Warning" ? YELLOW : RED})


Strictly, you would have to put each element between two shift operators on their own line. This will probably look weird, but please at least put the first << at the end of the first line.

chriseth · 2018-03-13T17:11:43Z

test/libsolidity/SyntaxTest.cpp

 }

 void SyntaxTest::printErrorList(
 	ostream& _stream,
 	ErrorList const& _errorList,
-	string const& _indent
+	string const& _linePrefix,
+	bool const _ignoreWarnings,


Can you use enums instead? I'm fine with named bit fields, too.

Are you sure it makes sense to define an enum/bit field for this single function alone?

Yes, because then you know what the bools mean.

chriseth · 2018-03-13T18:32:12Z

test/tools/isoltest.cpp

+	if (_parserError)
+		cout << "(e)dit/(s)kip/(q)uit? ";
+	else
+		cout << "(e)dit/(r)eplace/(s)kip/(q)uit? ";


Perhaps (u)pdate expectations instead of (r)eplace?

chriseth · 2018-03-13T18:33:03Z

test/tools/isoltest.cpp

+			return true;
+		case 'r':
+			cout << endl;
+			if (!_parserError)


What happens here in case of a parser error?

chriseth · 2018-03-13T18:33:58Z

test/tools/isoltest.cpp

+				if (!m_test->errorList().empty())
+				{
+					m_test->disableFormatting();
+					m_test->printErrorList(file, m_test->errorList(), "// ", false, false);


Wouldn't it be better to add a _formatting parameter to printErrorList?

chriseth · 2018-03-13T18:34:22Z

test/tools/isoltest.cpp

+						m_test->enableFormatting();
+				}
+			}
+			return process(_successCount);


Perhaps add a message "Re-running test case..."?

chriseth · 2018-03-13T18:36:06Z

test/tools/isoltest.cpp

+
+
+bool SyntaxTestTool::processPath(
+	fs::path const& _basepath,


Is it important to distinguish between _basepath and _path for this function?

I would say that the output of isoltest is more readable, if it prints paths relative to the base path and not absolute paths -

ekpyron · 2018-03-14T18:30:11Z

@chriseth I think this should be ready for another review now.

ekpyron · 2018-03-14T20:33:57Z

test/tools/isoltest.cpp

+			}
+		case 'e':
+			cout << endl << endl;
+			if (system((editor + " " + m_path.string()).c_str()))


system is the only possibility I know of that is part of the C++11 standard.
execv et al. would require a distinction between unix and windows.
boost::process is only included in boost >= 1.64.

We should at least quote the path.

chriseth · 2018-03-15T14:52:09Z

I noticed that non-bold cyan and yellow are not too well readable on my screen. Furthermore, perhaps we should not overdo it with the colours. For example, reading a full contract printed in colour is a little cumbersome.

What about colouring the words Contract, Expected result and Obtained result' instead? (and of course OK ERROR`, etc)

chriseth · 2018-03-15T15:03:36Z

test/tools/isoltest.cpp

+	{
+		success = m_test->run(outputMessages, "  ", m_formatted);
+	}
+	catch (...)


I think I asked that before, but under which circumstances can we have an exception here?

Ah nevermind, I found it. Can you please add a comment?

Hm, actually in the medium term, this tool should also be used for parser tests, in which case we need to use a different framework and thus would not get an exception here, but we can keep it like that for now.

Yes - in the medium term it would be best to use/write/modify a framework that gracefully returns both parser and syntax errors, instead of catching exceptions here.

ekpyron · 2018-03-15T15:14:02Z

I would still like to keep the colors for the results and expected results to distinguish optically between warnings and errors. I will change it to bold yellow and red and we can see whether that helps. The contract can be displayed in white, though, and instead the headings (Contract:, etc.) in maybe also bold cyan.

chriseth · 2018-03-15T15:15:40Z

test/tools/isoltest.cpp

+		{
+			cout << "  ";
+			FormattedScope(cout, m_formatted, {INVERSE, RED}) << "Parsing failed:" << endl;
+			m_test->printErrorList(cout, m_test->compilerErrors(), "    ", true, true, m_formatted);


Why are the warnings ignored here?

In case of a parser error the warnings don't really matter - it means the contract cannot be properly parsed due to some errors and the user is supposed to fix those errors by manually editing the contract - warnings would only be a distraction here.

This will, of course, change in future versions of the tool that includes parsing tests, etc.

Ok, we can keep it like that. The main reason I'm asking here is because if we do not need to ignore warning, we can get rid of one of the three bool parameters.

chriseth · 2018-03-15T15:16:36Z

@ekpyron what about keeping the colours only for the error type and not the whole message?

ekpyron · 2018-03-15T15:31:12Z

@chriseth That might be a good compromise. I first squashed bold colours and cyan for headings, white for the contract into the old commits and then added a new commit that only colours the error type, not the message.

I actually preferred the old colour scheme, but I'm fine with this as well (the actual result will largely depend on the used terminal emulator).

ekpyron · 2018-03-15T15:49:05Z

@chriseth I see only now that you approved the changes before the last commit (which changes the bool arguments for SyntaxTest::printErrorList to an enum). I actually prefer bool arguments in this particular case, since I don't think these arguments make up a meaningful enum (although in general, I agree that functions with lots of bool arguments should be avoided). Also the enum itself would be misplaced outside the SyntaxTest class or directly inside the SyntaxTest class as old-style enum, so I used an enum class as bitfield, which in turn means having to overload its operators...

In any case I'm fine with merging with or without this last change.

chriseth · 2018-03-15T16:02:19Z

Sorry for the mis-communication. I agree that in this particular case, we are probably better off without the enum, especially since we can hopefully remove one or two of the bools in the medium term.

For example, it might be useful to also include the line numbers into the expectations.

ekpyron mentioned this pull request Mar 12, 2018

[WIP] Infrastructure for extracting syntax tests. #3674

Closed

ekpyron self-assigned this Mar 12, 2018

ekpyron force-pushed the syntaxTestsInteractiveTool branch 10 times, most recently from 2a0d558 to b7b493e Compare March 13, 2018 16:02

ekpyron requested review from chriseth, axic and erak March 13, 2018 16:06

ekpyron force-pushed the syntaxTestsInteractiveTool branch from 06ee88b to 90975d3 Compare March 13, 2018 16:23

chriseth reviewed Mar 13, 2018

View reviewed changes

ekpyron force-pushed the syntaxTestsInteractiveTool branch 12 times, most recently from 41ff4d4 to cb59b7d Compare March 14, 2018 15:15

ekpyron changed the title ~~Interactive syntax test tool.~~ [WIP] Interactive syntax test tool. Mar 14, 2018

ekpyron force-pushed the syntaxTestsInteractiveTool branch 6 times, most recently from 254a1d3 to 9936faf Compare March 14, 2018 18:25

ekpyron changed the title ~~[WIP] Interactive syntax test tool.~~ Interactive syntax test tool. Mar 14, 2018

ekpyron force-pushed the syntaxTestsInteractiveTool branch 2 times, most recently from 796aeaa to c64baa4 Compare March 14, 2018 20:25

ekpyron commented Mar 14, 2018

View reviewed changes

ekpyron force-pushed the syntaxTestsInteractiveTool branch 3 times, most recently from 6948cbd to 79c46d1 Compare March 15, 2018 13:40

chriseth reviewed Mar 15, 2018

View reviewed changes

ekpyron added 2 commits March 15, 2018 16:23

Add formatted printing to SyntaxTest and expand its public interface.

269241e

Add interactive test tool isoltest.

7fa892e

ekpyron force-pushed the syntaxTestsInteractiveTool branch from 79c46d1 to 7fa892e Compare March 15, 2018 15:24

Only colour error type, not error message in isoltest.

50ad89d

chriseth approved these changes Mar 15, 2018

View reviewed changes

chriseth force-pushed the syntaxTestsInteractiveTool branch from a65b650 to 50ad89d Compare March 15, 2018 16:03

chriseth merged commit 9e10956 into develop Mar 15, 2018

ekpyron deleted the syntaxTestsInteractiveTool branch March 19, 2018 08:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Interactive syntax test tool. #3709

Interactive syntax test tool. #3709

ekpyron commented Mar 12, 2018

chriseth left a comment

chriseth Mar 13, 2018

chriseth Mar 13, 2018

chriseth Mar 13, 2018

chriseth Mar 13, 2018

chriseth Mar 13, 2018

chriseth Mar 13, 2018

ekpyron Mar 14, 2018

chriseth Mar 15, 2018

chriseth Mar 13, 2018

chriseth Mar 13, 2018

chriseth Mar 13, 2018

chriseth Mar 13, 2018

chriseth Mar 13, 2018

ekpyron Mar 14, 2018

ekpyron commented Mar 14, 2018

ekpyron Mar 14, 2018

chriseth Mar 15, 2018

chriseth commented Mar 15, 2018

chriseth Mar 15, 2018

chriseth Mar 15, 2018

chriseth Mar 15, 2018

ekpyron Mar 15, 2018

ekpyron commented Mar 15, 2018

chriseth Mar 15, 2018

ekpyron Mar 15, 2018

chriseth Mar 15, 2018

chriseth commented Mar 15, 2018

ekpyron commented Mar 15, 2018

ekpyron commented Mar 15, 2018

chriseth commented Mar 15, 2018

Interactive syntax test tool. #3709

Interactive syntax test tool. #3709

Conversation

ekpyron commented Mar 12, 2018

chriseth 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekpyron commented Mar 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseth commented Mar 15, 2018

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

ekpyron commented Mar 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseth commented Mar 15, 2018

ekpyron commented Mar 15, 2018

ekpyron commented Mar 15, 2018

chriseth commented Mar 15, 2018