Decouple transfer tests from GCS using local CSV fixtures#391
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes the GCS (Google Cloud Storage) dependency from transfer tests by enabling the use of local CSV fixtures instead. The changes allow test_contact_with_multiple_wells to run entirely on local data, improving test isolation and eliminating external dependencies.
Changes:
- Added
_read_csvmethod to the baseTransfererclass that respectsCSV_PATHSflag for local file loading - Updated
WellTransfererandContactTransferto use_read_csvinstead of directread_csvcalls - Enhanced
_make_emailand_make_phoneto handle non-string values safely by coercing to strings before stripping
Reviewed changes
Copilot reviewed 4 out of 8 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| transfers/transferer.py | Added base _read_csv method that checks CSV_PATHS flag and falls back to GCS |
| transfers/well_transfer.py | Updated to use _read_csv and conditionally initialize cached elevations based on CSV_PATHS flag |
| transfers/contact_transfer.py | Updated to use _read_csv and added null/type safety for email and phone processing |
| tests/transfers/test_contact_with_multiple_wells.py | Modified test to use local CSV fixtures via CSV_PATHS flag instead of GCS |
| if self.flags.get("CSV_PATHS"): | ||
| self._cached_elevations = {} | ||
| else: | ||
| self._cached_elevations = get_cached_elevations() |
There was a problem hiding this comment.
The conditional initialization of _cached_elevations lacks an explanatory comment. It's not immediately clear why CSV_PATHS disables cached elevations. Consider adding a comment explaining that local CSV mode bypasses the GCS elevation cache to avoid external dependencies during testing.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9de4f1ac88
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| input_df = read_csv(self.source_table, self.source_dtypes) | ||
| input_df = self._read_csv(self.source_table, self.source_dtypes) |
There was a problem hiding this comment.
When CSV_PATHS is used, _read_csv routes through pd.read_csv, so the positional self.source_dtypes here is interpreted as the sep argument instead of dtype. For chunk transferers that set source_dtypes to a dict (e.g., LinkIdsWellDataTransferer), this will raise a delimiter error or parse the CSV incorrectly, which breaks any local-fixture run with CSV_PATHS. Pass dtype=self.source_dtypes (or update _read_csv to accept a dtype parameter explicitly) to keep local CSVs readable.
Useful? React with 👍 / 👎.
| def __init__(self, *args, **kw): | ||
| super().__init__(*args, **kw) | ||
| self._cached_elevations = get_cached_elevations() | ||
| if self.flags.get("CSV_PATHS"): | ||
| self._cached_elevations = {} | ||
| else: |
There was a problem hiding this comment.
With CSV_PATHS enabled, _cached_elevations is now initialized as an empty dict, but WellTransferer._after_hook still unconditionally calls dump_cached_elevations to GCS. This means a local/fixture run will overwrite the shared cached_elevations.json with only the subset encountered in the fixture (or fail if GCS credentials are unavailable), which defeats the goal of decoupling tests from GCS and can corrupt the cache used by other runs. Consider skipping the dump or merging with the existing cache when CSV_PATHS is set.
Useful? React with 👍 / 👎.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| if self.flags.get("CSV_PATHS"): | ||
| self._cached_elevations = {} | ||
| else: | ||
| self._cached_elevations = get_cached_elevations() |
There was a problem hiding this comment.
The conditional initialization of _cached_elevations creates two different behaviors based on the CSV_PATHS flag. When using local CSVs, the empty dictionary bypasses elevation caching entirely. Consider adding a comment explaining why elevation caching is disabled for local CSV paths, or ensure that the elevation logic properly handles the empty cache case without attempting GCS operations.
| "first", | ||
| row.OwnerKey, | ||
| email=row.Email.strip(), | ||
| email=str(row.Email).strip() if row.Email is not None else None, |
There was a problem hiding this comment.
The inline type coercion and null check could be simplified by extracting this logic into a helper function, especially since similar patterns appear in _make_email and _make_phone. This would improve consistency and reduce duplication across the codebase.
TylerAdamMartinez
deleted the
remove-gcs-dependency-from-transfer-test
branch
Why
How
Notes
Tests