Merge pull request #71 from FlickerSoul/main

Fix: loading context values from student submissions instead of answer files

docs/advanced.md

@@ -123,20 +123,20 @@ The `case` exposes `args` arguments and `kwargs` variables which are passed from
 The `case` also exposes `name` and `description` variables which are the name of the test case and the description of the test case. Changing those variables is equivalent to changing `aga_name` and `aga_description` but this means you can set it dynamically during the testing. 
 
 
-## Capture Environment Values 
+## Capture Context Values 
 
-Sometimes a piece of assignment file includes multiple classes, and even though only one class is eventually tested, the other parts of students' answers can be crucial. For example, consider the following file. You can specify in the `envs` argument of `problem` decorator to capture the `GasStation` class, and in the override check function, you can reference the `GasStation` class in the student's answer. 
+Sometimes a piece of assignment file includes multiple classes, and even though only one class is eventually tested, the other parts of students' answers can be crucial. For example, consider the following file. You can specify in the `ctx` argument of `problem` decorator to capture the `GasStation` class, and in the override check function, you can reference the `GasStation` class in the student's answer. 
 
 ```python
 from aga import problem, test_case
 
 def override_check(case, golden, student):
-    # use case.env.GasStation to reference student's GasStation class implementation
+    # use case.ctx.GasStation to reference student's GasStation class implementation
     ...
 
 
 @test_case(aga_override_check=override_check)
-@problem(envs=['GasStation'])
+@problem(ctx=['GasStation'])
 class Car:
     # uses gas station somewhere in the code
     ...
@@ -145,4 +145,6 @@ class GasStation:
     ...
 ```
 
-Essentially, `envs` argument takes in an iterable of strings, and aga will search the corresponding fields in the students' submitted module (file). 
+Essentially, `ctx` argument takes in an iterable of strings, and aga will search the corresponding fields in the students' submitted module (file). 
+
+Note that `ctx` is should not be modified during overriden check functions, since the changes will persist to all the following test cases, which might not be the intended behavior.

pyproject.toml

@@ -1,6 +1,6 @@
 [tool.poetry]
 name = "aga"
-version = "0.13.9"
+version = "0.13.10"
 description = "aga grades assignments"
 authors = ["Riley Shahar <riley.shahar@gmail.com>"]
 license = "MIT"

src/aga/config.py

@@ -23,6 +23,8 @@ def _default_value(path: list[str]) -> Any:
 
 def _from_default(path: list[str]) -> Any:
     """Given a path of config names, construct a field which inherits the default."""
+    # pylint: disable=invalid-field-call
+    # this is necessary for the default_factory
     return field(default_factory=lambda: _default_value(path))  # type: ignore
 
 

src/aga/core/environment.py → src/aga/core/context.py

@@ -1,26 +1,25 @@
 """Environment value wrapper."""
 from __future__ import annotations
 
-import types
 from typing import Iterable, Any
 
 
-class Environment(dict[str, Any]):
+class SubmissionContext(dict[str, Any]):
     """Environment value wrapper."""
 
     def __init__(self, env_targets: Iterable[str]) -> None:
         super().__init__()
         for target in env_targets:
             self[target] = None
 
-    def update_from_module(self, mod: types.ModuleType) -> None:
+    def update_from_path(self, path: str) -> None:
         """Update environment values from a given module."""
-        for value_name in self.keys():
-            if value_name not in vars(mod):
-                raise ValueError(
-                    f"The variable `{value_name} does not exist in the module {mod}"
-                )
-            self[value_name] = getattr(mod, value_name)
+        # pylint: disable=import-outside-toplevel, cyclic-import
+        # to avoid circular imports, I place the import here
+        from ..loader import load_symbol_from_path
+
+        for symbol in self.keys():
+            self[symbol] = load_symbol_from_path(path, symbol)
 
     def __getattr__(self, item: str) -> Any:
         """Get the value of an environment variable."""

src/aga/core/parameter.py

@@ -208,6 +208,7 @@ class _TestParam(AgaKeywordContainer):
 
     pipeline: ClassVar[partial[_TestParam]]
 
+    # pylint: disable=too-many-arguments
     @overload
     def __init__(
         self,
@@ -383,7 +384,7 @@ class _TestParams:
     product: ClassVar[partial[_TestParams]]
     singular_params: ClassVar[partial[_TestParams]]
 
-    # pylint: disable=too-many-locals
+    # pylint: disable=too-many-locals, too-many-arguments
     @overload
     def __init__(
         self,

src/aga/core/problem.py

@@ -1,7 +1,6 @@
 """Problem and utilities."""
 from __future__ import annotations
 
-from types import ModuleType
 from typing import (
     TYPE_CHECKING,
     Callable,
@@ -14,7 +13,7 @@
 
 from ..config import AgaConfig
 from ..score import Prize, ScoredPrize, compute_scores
-from .environment import Environment
+from .context import SubmissionContext
 from .suite import AgaTestSuite, SubmissionMetadata, _TestInputGroup, _TestInputs
 
 # pylint: disable=invalid-name
@@ -39,15 +38,15 @@ def __init__(
         name: str,
         config: AgaConfig,
         is_script: bool,
-        env_targets: Iterable[str] = (),
+        ctx_targets: Iterable[str] = (),
     ) -> None:
         self._golden: Callable[ProblemParamSpec, ProblemOutputType] = golden
         self._name = name
         self._config = config
         self._ungrouped_prizes: list[Prize] = []
         self._ungrouped_tests: list[_TestInputs[ProblemOutputType]] = []
         self._groups: list[_TestInputGroup[ProblemOutputType]] = []
-        self._environment: Environment = Environment(env_targets)
+        self._submission_context: SubmissionContext = SubmissionContext(ctx_targets)
         self.is_script = is_script
 
     def add_test_case(self, param: _TestParam) -> None:
@@ -57,7 +56,9 @@ def add_test_case(self, param: _TestParam) -> None:
         does _not_ produce a test of the golden solution.
         """
         case: _TestInputs[ProblemOutputType] = _TestInputs(
-            param, mock_input=self._config.problem.mock_input, env=self.environment
+            param,
+            mock_input=self._config.problem.mock_input,
+            ctx=self.submission_context,
         )
         self._ungrouped_tests.append(case)
 
@@ -143,16 +144,9 @@ def name(self) -> str:
         return self._name
 
     @property
-    def environment(self) -> Environment:
+    def submission_context(self) -> SubmissionContext:
         """The environment values captured from the problem module."""
-        return self._environment
-
-    def update_env_from(
-        self, mod: ModuleType
-    ) -> Problem[ProblemParamSpec, ProblemOutputType]:
-        """Update the environment values from a given module."""
-        self.environment.update_from_module(mod)
-        return self
+        return self._submission_context
 
     def expected_symbol(self) -> str:
         """Get the name of the symbol that should be tested against."""
@@ -194,7 +188,7 @@ def problem(
     script: bool = False,
     check_stdout: Optional[bool] = None,
     mock_input: Optional[bool] = None,
-    envs: Iterable[str] = (),
+    ctx: Iterable[str] = (),
 ) -> Callable[
     [Callable[ProblemParamSpec, ProblemOutputType]],
     Problem[ProblemParamSpec, ProblemOutputType],
@@ -221,8 +215,8 @@ def problem(
         Overrides the `problem.mock_input` configuration option. If True, test cases for
         this problem will be interpreted as mocked outputs of `builtins.input`, rather
         than inputs to the function.
-    envs : Iterable[str]
-        The environment values to be captured
+    ctx: Iterable[str]
+        The context values required in the submission and will be captured
 
     Returns
     -------
@@ -253,7 +247,7 @@ def outer(
                 config.problem.mock_input = True
                 config.problem.mock_input_overridden = True
 
-        return Problem(func, problem_name, config, script, env_targets=envs)
+        return Problem(func, problem_name, config, script, ctx)
 
     return outer
 

src/aga/core/suite.py

@@ -9,7 +9,7 @@
 from unittest import TestCase, TestSuite
 from unittest.mock import patch
 
-from aga.core.environment import Environment
+from aga.core.context import SubmissionContext
 
 from ..config import AgaConfig, AgaTestConfig
 from ..score import Prize, ScoredPrize, ScoreInfo, compute_scores
@@ -154,11 +154,11 @@ def __init__(
         self,
         aga_param: _TestParam,
         mock_input: bool,
-        env: Environment | None = None,
+        ctx: SubmissionContext | None = None,
     ) -> None:
         super().__init__()
         self._mock_input = mock_input
-        self.env = env
+        self.ctx = ctx
         self._param: _TestParam = aga_param
         self.score_info = ScoreInfo(
             self.aga_kwargs.weight, self.aga_kwargs.value, self.aga_kwargs.extra_credit

src/aga/loader.py

@@ -44,6 +44,14 @@ class MultipleScripts(InvalidSubmissionError):
     """Too many scripts were found."""
 
 
+class ContextMissing(InvalidSubmissionError):
+    """The submission file does not contain the values required by the environment."""
+
+
+class AmbiguousContext(InvalidSubmissionError):
+    """The submission files have  multiple values for the same environment variable."""
+
+
 def _get_spec_from_path(path: str, name: str) -> ModuleSpec:
     """Get the spec of the module at path."""
     spec = importlib.util.spec_from_file_location(name, path)
@@ -132,11 +140,8 @@ def _load_from_module_by(
 
 def _load_problems_from_module(module: ModuleType) -> Iterable[Problem[Any, Any]]:
     """Return all problems in the module."""
-    yield from (
-        prob.update_env_from(module)
-        for prob in _load_from_module_by(
-            lambda i: isinstance(i, Problem), module  # type: ignore
-        )
+    yield from _load_from_module_by(
+        lambda i: isinstance(i, Problem), module  # type: ignore
     )
 
 
@@ -156,16 +161,20 @@ def _load_symbol_from_dir(path: str, symbol: str) -> Any:
     """Load a specific symbol from any of the source files in a directory."""
     matching_symbols = []
     for file in os.listdir(path):
+        # ignore the pycache folder to avoid duplicated symbols
+        if file == "__pycache__":
+            continue
+
         try:
             file_path = pathjoin(path, file)
             matching_symbols.append(load_symbol_from_path(file_path, symbol))
         except (FileNotFoundError, NoMatchingSymbol):
             pass
 
     if len(matching_symbols) > 1:
-        raise TooManyMatchingSymbols
+        raise TooManyMatchingSymbols(f"Multiple matching symbols {symbol} found.")
     if len(matching_symbols) == 0:
-        raise NoMatchingSymbol
+        raise NoMatchingSymbol(f"No matching symbol {symbol} found.")
     return matching_symbols[0]
 
 

src/aga/runner.py

@@ -7,7 +7,6 @@
 For convenience, it also provides the `load_and_run` method, which loads a student
 submission and then runs it.
 """
-
 from dataclasses import dataclass
 from typing import Any, Literal, Optional, TypeVar
 from unittest import TestResult
@@ -23,6 +22,8 @@
     TooManyMatchingSymbols,
     load_script_from_path,
     load_symbol_from_path,
+    ContextMissing,
+    AmbiguousContext,
 )
 from .score import ScoredPrize
 from .util import limited_traceback
@@ -312,6 +313,20 @@ def load_and_run(
             score=0.0,
         )
 
+    if not problem.is_script:
+        # If the submission is a module, we need to update the required context
+        # with the values from the submission.
+        try:
+            problem.submission_context.update_from_path(path)
+        except NoMatchingSymbol as e:
+            raise ContextMissing(
+                "The submission does not include some required context"
+            ) from e
+        except TooManyMatchingSymbols as e:
+            raise AmbiguousContext(
+                "The submission includes multiple values for the same context"
+            ) from e
+
     suite, prizes = problem.generate_test_suite(under_test, metadata)
     return _run(suite, prizes, metadata)
 

tests/conftest.py

@@ -10,7 +10,7 @@
 from os.path import join as pathjoin
 from pathlib import Path
 from shutil import copyfileobj
-from typing import Callable, Generator, Iterable, Iterator, List
+from typing import Callable, Generator, Iterable, Iterator, List, Any, Type
 from unittest import TestCase
 
 import pytest
@@ -156,6 +156,18 @@ def __init__(self):
 
     def adder(self, x: int) -> int:
         return self.x + self.y + x
+""",
+    "test_context_loading": """
+class GasTank:
+    pass
+class Car:
+    def __init__(self, tank: GasTank):
+        self.tank = tank
+""",
+    "test_no_context_values": """
+class Car:
+    def __init__(self, tank):
+        self.tank = tank
 """,
 }
 
@@ -1187,3 +1199,37 @@ class TestObj(_TestObj):
         """A test object for testing."""
 
     return TestObj  # type: ignore
+
+
+@pytest.fixture(name="test_context_loading")
+def fixture_test_context_loading() -> Problem[[], Any]:
+    """Generate a test requiring context values."""
+
+    def override_test(
+        tc: _TestInputs[Car], golden: Type[Car], student: Type[Car]
+    ) -> None:
+        """Check if test_case's context has GasTank."""
+        # GasTank should be run without problem
+        assert tc.ctx is not None
+        tank = tc.ctx["GasTank"]
+        golden_car = golden(tank)
+        student_car = student(tank)
+        assert golden_car.tank == student_car.tank
+
+    class GasTank:
+        """A gas tank needed by the Car."""
+
+    @test_case(aga_override_test=override_test)
+    @problem(ctx=["GasTank"])
+    class Car:
+        """A car with a gas tank."""
+
+        def __init__(self, tank: GasTank) -> None:
+            """Initialize the car."""
+            self.tank = tank
+
+    # pylint: disable=no-member
+    # since Car is transformed into a Problem by the decorator
+    assert Car.submission_context.GasTank is None  # type: ignore
+
+    return Car  # type: ignore