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 <neubig@gmail.com>

.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:

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'

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<Props> = ({ name }) => {
+  const [count, setCount] = useState(0);
+
+  return (
+    <div>
+      <h1>Hello, {name}!</h1>
+      <p>You clicked {count} times</p>
+      <button onClick={() => setCount(count + 1)}>
+        Click me
+      </button>
+    </div>
+  );
+};
+
+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 (
+    <div>
+      <h1>Hello, {props.name}!</h1>
+      <button onClick={() => console.log('Clicked')}>
+        Click me
+      </button>
+    </div>
+  );
+};
+
+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 (
+    <div className={`App ${darkMode ? 'dark-mode' : ''}`}>
+      <button onClick={toggleDarkMode}>
+        {darkMode ? 'Light Mode' : 'Dark Mode'}
+      </button>
+    </div>
+  )
+}
+
+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'