From 8fca5a53542e7963af9345d321ee1c5664cd396a Mon Sep 17 00:00:00 2001 From: tobitege <10787084+tobitege@users.noreply.github.com> Date: Thu, 29 Aug 2024 10:40:43 +0200 Subject: [PATCH] linter and test_aider_linter extensions for eslint (#3543) * linter and test_aider_linter extensions for eslint * linter tweaks * try enabling verbose output in linter test * one more option for linter test * try conftest.py for tests/unit folder * enable verbose mode in workflow; remove conftest.py again * debug print statements of linter results * skip some tests if eslint is not installed at all * more tweaks * final test skip setups * code quality revisions * fix test again --------- Co-authored-by: Graham Neubig --- .github/workflows/py-unit-tests.yml | 2 +- .../agent_skills/utils/aider/linter.py | 97 ++++++- tests/unit/test_aider_linter.py | 263 +++++++++++++++++- 3 files changed, 351 insertions(+), 11 deletions(-) diff --git a/.github/workflows/py-unit-tests.yml b/.github/workflows/py-unit-tests.yml index 3e95d7ab25..ec66b9a36b 100644 --- a/.github/workflows/py-unit-tests.yml +++ b/.github/workflows/py-unit-tests.yml @@ -111,7 +111,7 @@ jobs: - name: Build Environment run: make build - name: Run Tests - run: poetry run pytest --forked --cov=agenthub --cov=openhands --cov-report=xml ./tests/unit + run: poetry run pytest --forked --cov=agenthub --cov=openhands --cov-report=xml -svv ./tests/unit - name: Upload coverage to Codecov uses: codecov/codecov-action@v4 env: diff --git a/openhands/runtime/plugins/agent_skills/utils/aider/linter.py b/openhands/runtime/plugins/agent_skills/utils/aider/linter.py index 16768ee85e..2ee17f6cb4 100644 --- a/openhands/runtime/plugins/agent_skills/utils/aider/linter.py +++ b/openhands/runtime/plugins/agent_skills/utils/aider/linter.py @@ -1,10 +1,13 @@ +import json import os import subprocess import sys +import tempfile import traceback import warnings from dataclasses import dataclass from pathlib import Path +from typing import Optional from grep_ast import TreeContext, filename_to_lang from tree_sitter_languages import get_parser # noqa: E402 @@ -24,13 +27,19 @@ class Linter: self.encoding = encoding self.root = root + self.ts_installed = self._check_tool_installed('tsc') + self.eslint_installed = self._check_tool_installed('eslint') + self.languages = dict( python=self.py_lint, - javascript=self.ts_lint, - typescript=self.ts_lint, ) + if self.eslint_installed: + self.languages['javascript'] = self.ts_eslint + self.languages['typescript'] = self.ts_eslint + elif self.ts_installed: + self.languages['javascript'] = self.ts_tsc_lint + self.languages['typescript'] = self.ts_tsc_lint self.all_lint_cmd = None - self.ts_installed = self.check_ts_installed() def set_linter(self, lang, cmd): if lang: @@ -118,11 +127,11 @@ class Linter: error = basic_lint(rel_fname, code) return error - def check_ts_installed(self): - """Check if TypeScript is installed.""" + def _check_tool_installed(self, tool_name: str) -> bool: + """Check if a tool is installed.""" try: subprocess.run( - ['tsc', '--version'], + [tool_name, '--version'], check=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE, @@ -131,7 +140,81 @@ class Linter: except (subprocess.CalledProcessError, FileNotFoundError): return False - def ts_lint(self, fname, rel_fname, code): + def print_lint_result(self, lint_result: LintResult) -> None: + print(f'\n{lint_result.text.strip()}') + if isinstance(lint_result.lines, list) and lint_result.lines: + if isinstance(lint_result.lines[0], LintResult): + self.print_lint_result(lint_result.lines[0]) + + def ts_eslint(self, fname: str, rel_fname: str, code: str) -> Optional[LintResult]: + """Use ESLint to check for errors. If ESLint is not installed return None.""" + if not self.eslint_installed: + return None + + # Enhanced ESLint configuration with React support + eslint_config = { + 'env': {'es6': True, 'browser': True, 'node': True}, + 'extends': ['eslint:recommended', 'plugin:react/recommended'], + 'parserOptions': { + 'ecmaVersion': 2021, + 'sourceType': 'module', + 'ecmaFeatures': {'jsx': True}, + }, + 'plugins': ['react'], + 'rules': { + 'no-unused-vars': 'warn', + 'no-console': 'off', + 'react/prop-types': 'warn', + 'semi': ['error', 'always'], + }, + 'settings': {'react': {'version': 'detect'}}, + } + + # Write config to a temporary file + with tempfile.NamedTemporaryFile( + mode='w', suffix='.json', delete=False + ) as temp_config: + json.dump(eslint_config, temp_config) + temp_config_path = temp_config.name + + try: + # Point to frontend node_modules directory + if self.root: + plugin_path = f'{self.root}/frontend/node_modules/' + else: + return None + + eslint_cmd = f'eslint --no-eslintrc --config {temp_config_path} --resolve-plugins-relative-to {plugin_path} --format json' + eslint_res = '' + try: + eslint_res = self.run_cmd(eslint_cmd, rel_fname, code) + if eslint_res and hasattr(eslint_res, 'text'): + # Parse the ESLint JSON output + eslint_output = json.loads(eslint_res.text) + error_lines = [] + error_messages = [] + for result in eslint_output: + for message in result.get('messages', []): + line = message.get('line', 0) + error_lines.append(line) + error_messages.append( + f"{rel_fname}:{line}:{message.get('column', 0)}: {message.get('message')} ({message.get('ruleId')})" + ) + if not error_messages: + return None + + return LintResult(text='\n'.join(error_messages), lines=error_lines) + except json.JSONDecodeError as e: + return LintResult(text=f'\nJSONDecodeError: {e}', lines=[eslint_res]) + except FileNotFoundError: + return None + except Exception as e: + return LintResult(text=f'\nUnexpected error: {e}', lines=[]) + finally: + os.unlink(temp_config_path) + return None + + def ts_tsc_lint(self, fname, rel_fname, code): """Use typescript compiler to check for errors. If TypeScript is not installed return None.""" if self.ts_installed: tsc_cmd = 'tsc --noEmit --allowJs --checkJs --strict --noImplicitAny --strictNullChecks --strictFunctionTypes --strictBindCallApply --strictPropertyInitialization --noImplicitThis --alwaysStrict' diff --git a/tests/unit/test_aider_linter.py b/tests/unit/test_aider_linter.py index 247493a551..b3126c8158 100644 --- a/tests/unit/test_aider_linter.py +++ b/tests/unit/test_aider_linter.py @@ -1,11 +1,22 @@ import os -from unittest.mock import patch +from unittest.mock import MagicMock, patch import pytest from openhands.runtime.plugins.agent_skills.utils.aider import Linter, LintResult +def get_parent_directory(levels=3): + current_file = os.path.abspath(__file__) + parent_directory = current_file + for _ in range(levels): + parent_directory = os.path.dirname(parent_directory) + return parent_directory + + +print(f'\nRepo root folder: {get_parent_directory()}\n') + + @pytest.fixture def temp_file(tmp_path): # Fixture to create a temporary file @@ -107,6 +118,82 @@ foo(); os.remove(temp_name) +@pytest.fixture +def temp_typescript_file_eslint_pass(tmp_path): + temp_name = tmp_path / 'lint-test-pass.ts' + temp_name.write_text(""" +function greet(name: string): void { + console.log(`Hello, ${name}!`); +} +greet("World"); +""") + return str(temp_name) + + +@pytest.fixture +def temp_typescript_file_eslint_fail(tmp_path): + temp_name = tmp_path / 'lint-test-fail.ts' + temp_name.write_text(""" +function greet(name) { + console.log("Hello, " + name + "!") + var unused = "This variable is never used"; +} +greet("World") +""") + return str(temp_name) + + +@pytest.fixture +def temp_react_file_pass(tmp_path): + temp_name = tmp_path / 'react-component-pass.tsx' + temp_name.write_text(""" +import React, { useState } from 'react'; + +interface Props { + name: string; +} + +const Greeting: React.FC = ({ name }) => { + const [count, setCount] = useState(0); + + return ( +
+

Hello, {name}!

+

You clicked {count} times

+ +
+ ); +}; + +export default Greeting; +""") + return str(temp_name) + + +@pytest.fixture +def temp_react_file_fail(tmp_path): + temp_name = tmp_path / 'react-component-fail.tsx' + temp_name.write_text(""" +import React from 'react'; + +const Greeting = (props) => { + return ( +
+

Hello, {props.name}!

+ +
+ ); +}; + +export default Greeting; +""") + return str(temp_name) + + def test_get_rel_fname(linter, temp_file, tmp_path): # Test get_rel_fname method rel_fname = linter.get_rel_fname(temp_file) @@ -246,8 +333,9 @@ def test_lint_fail_ruby_no_parentheses(linter, temp_ruby_file_errors_parentheses def test_lint_pass_typescript(linter, temp_typescript_file_correct): if linter.ts_installed: - result = linter.lint(temp_typescript_file_correct) - assert result is None + with patch.object(linter, 'root', return_value=get_parent_directory()): + result = linter.lint(temp_typescript_file_correct) + assert result is None def test_lint_fail_typescript(linter, temp_typescript_file_errors): @@ -263,3 +351,172 @@ def test_lint_fail_typescript_missing_semicolon( with patch.dict(os.environ, {'ENABLE_AUTO_LINT': 'True'}): errors = linter.lint(temp_typescript_file_errors_semicolon) assert errors is not None + + +def test_ts_eslint_pass(linter, temp_typescript_file_eslint_pass): + with patch.object(linter, 'eslint_installed', return_value=True): + with patch.object(linter, 'root', return_value=get_parent_directory()): + with patch.object(linter, 'run_cmd') as mock_run_cmd: + mock_run_cmd.return_value = MagicMock(text='[]') # Empty ESLint output + result = linter.ts_eslint( + temp_typescript_file_eslint_pass, 'lint-test-pass.ts', '' + ) + assert result is None # No lint errors expected + + +def test_ts_eslint_not_installed(linter, temp_typescript_file_eslint_pass): + with patch.object(linter, 'eslint_installed', return_value=False): + with patch.object(linter, 'root', return_value=get_parent_directory()): + result = linter.lint(temp_typescript_file_eslint_pass) + assert result is None # Should return None when ESLint is not installed + + +def test_ts_eslint_run_cmd_error(linter, temp_typescript_file_eslint_pass): + with patch.object(linter, 'eslint_installed', return_value=True): + with patch.object(linter, 'run_cmd', side_effect=FileNotFoundError): + result = linter.ts_eslint( + temp_typescript_file_eslint_pass, 'lint-test-pass.ts', '' + ) + assert result is None # Should return None when run_cmd raises an exception + + +def test_ts_eslint_react_pass(linter, temp_react_file_pass): + if not linter.eslint_installed: + pytest.skip('ESLint is not installed. Skipping this test.') + + with patch.object(linter, 'eslint_installed', return_value=True): + with patch.object(linter, 'run_cmd') as mock_run_cmd: + mock_run_cmd.return_value = MagicMock(text='[]') # Empty ESLint output + result = linter.ts_eslint( + temp_react_file_pass, 'react-component-pass.tsx', '' + ) + assert result is None # No lint errors expected + + +def test_ts_eslint_react_fail(linter, temp_react_file_fail): + if not linter.eslint_installed: + pytest.skip('ESLint is not installed. Skipping this test.') + + with patch.object(linter, 'run_cmd') as mock_run_cmd: + mock_eslint_output = """[ + { + "filePath": "react-component-fail.tsx", + "messages": [ + { + "ruleId": "react/prop-types", + "severity": 1, + "message": "Missing prop type for 'name'", + "line": 5, + "column": 22, + "nodeType": "Identifier", + "messageId": "missingPropType", + "endLine": 5, + "endColumn": 26 + }, + { + "ruleId": "no-console", + "severity": 1, + "message": "Unexpected console statement.", + "line": 7, + "column": 29, + "nodeType": "MemberExpression", + "messageId": "unexpected", + "endLine": 7, + "endColumn": 40 + } + ], + "errorCount": 0, + "warningCount": 2, + "fixableErrorCount": 0, + "fixableWarningCount": 0, + "source": "..." + } + ]""" + mock_run_cmd.return_value = MagicMock(text=mock_eslint_output) + linter.root = get_parent_directory() + result = linter.ts_eslint(temp_react_file_fail, 'react-component-fail.tsx', '') + if not linter.eslint_installed: + assert result is None + return + + assert isinstance(result, LintResult) + assert ( + "react-component-fail.tsx:5:22: Missing prop type for 'name' (react/prop-types)" + in result.text + ) + assert ( + 'react-component-fail.tsx:7:29: Unexpected console statement. (no-console)' + in result.text + ) + assert 5 in result.lines + assert 7 in result.lines + + +def test_ts_eslint_react_config(linter, temp_react_file_pass): + if not linter.eslint_installed: + pytest.skip('ESLint is not installed. Skipping this test.') + + with patch.object(linter, 'root', return_value=get_parent_directory()): + with patch.object(linter, 'run_cmd') as mock_run_cmd: + mock_run_cmd.return_value = MagicMock(text='[]') # Empty ESLint output + linter.root = get_parent_directory() + result = linter.ts_eslint( + temp_react_file_pass, 'react-component-pass.tsx', '' + ) + assert result is None + # Check if the ESLint command includes React-specific configuration + called_cmd = mock_run_cmd.call_args[0][0] + assert 'resolve-plugins-relative-to' in called_cmd + # Additional assertions to ensure React configuration is present + assert '--config /tmp/' in called_cmd + + +def test_ts_eslint_react_missing_semicolon(linter, tmp_path): + if not linter.eslint_installed: + pytest.skip('ESLint is not installed. Skipping this test.') + + temp_react_file = tmp_path / 'App.tsx' + temp_react_file.write_text("""import React, { useState, useEffect, useCallback } from 'react' +import './App.css' + +function App() { + const [darkMode, setDarkMode] = useState(false); + + const toggleDarkMode = () => { + setDarkMode(!darkMode); + document.body.classList.toggle('dark-mode'); + }; + + return ( +
+ +
+ ) +} + +export default App +""") + + linter.root = get_parent_directory() + result = linter.ts_eslint(str(temp_react_file), str(temp_react_file), '') + assert isinstance(result, LintResult) + + if 'JSONDecodeError' in result.text: + linter.print_lint_result(result) + pytest.skip( + 'ESLint returned a JSONDecodeError. This might be due to a configuration issue.' + ) + + if 'eslint-plugin-react' in result.text and "wasn't found" in result.text: + linter.print_lint_result(result) + pytest.skip( + 'eslint-plugin-react is not installed. This test requires the React ESLint plugin.' + ) + + assert any( + 'Missing semicolon' in message for message in result.text.split('\n') + ), "Expected 'Missing semicolon' error not found" + assert 1 in result.lines, 'Expected line 1 to be flagged for missing semicolon' + assert 21 in result.lines, 'Expected line 21 to be flagged for missing semicolon'