Skip to content

Conversation

@niffy-artist2876
Copy link

@niffy-artist2876 niffy-artist2876 commented Sep 8, 2025

#123 - Introduced a method such that the CSV files have a better naming convention and a separate folder for saving the CSV files

📌 Description

Please provide a concise summary of the changes:

  • What is the purpose of this PR?
  • What problem does it solve, or what feature does it add?
  • Any relevant motivation, background, or context?

ℹ️ Fixes / Related Issues
Fixes: #100
Related: #1

🧱 Type of Change

Please indicate the type of changes introduced in your PR. Anything left unchecked will be assumed to be non-relevant

  • 🐛 Bug fix – Non-breaking fix for a functional/logic error
  • [✅] ✨ New feature – Adds functionality without breaking existing APIs
  • ⚠️ Breaking change – Introduces backward-incompatible changes (API, schema, etc.)
  • 📝 Documentation update – README, docstrings, OpenAPI tags, etc.
  • 🧪 Test suite change – Adds/updates unit, functional, or integration tests
  • ⚙️ CI/CD pipeline update – Modifies GitHub Actions, pre-commit, or Docker build
  • 🧹 Code quality / Refactor – Improves structure, readability, or style (no functional changes)
  • 🐢 Performance improvement – Speeds up auth, scraping, or reduces I/O
  • 🕵️ Debug/logging enhancement – Adds or improves logging/debug support
  • 🔧 Developer tooling – Scripts, benchmarks, local testing improvements
  • 🔒 Security fix – Addresses auth/session/data validation vulnerabilities
  • 🧰 Dependency update – Updates libraries in requirements.txt, pyproject.toml

🧪 How Has This Been Tested?

Please indicate how you tested your changes. Completing all the relevant items on this list is mandatory

  • Unit Tests (tests/unit/)
  • Functional Tests (tests/functional/)
  • Integration Tests (tests/integration/)
  • Manual Testing

⚙️ Test Configuration:

  • OS: (e.g., Linux)
  • Python: (e.g., 3.12 via uv)
  • Docker build tested

✅ Checklist

Please indicate the work items you have carried out. Completing all the relevant items on this list is mandatory. Anything left unchecked will be assumed to be non-relevant

  • [✅] My code follows the CONTRIBUTING.md guidelines
  • [✅] I've performed a self-review of my changes
  • I've added/updated necessary comments and docstrings
  • I've updated relevant docs (README or endpoint docs)
  • No new warnings introduced
  • I've added tests to cover my changes
  • [✅] All tests pass locally (scripts/run_tests.py)
  • [✅] I've run linting and formatting (pre-commit run --all-files)
  • Docker image builds and runs correctly
  • Changes are backwards compatible (if applicable)
  • Feature flags or .env vars updated (if applicable)
  • I've tested across multiple environments (if applicable)
  • Benchmarks still meet expected performance (scripts/benchmark/benchmark_requests.py)

🛠️ Affected API Behaviour

Please indicate the areas affected by changes introduced in your PR

  • app/app.py – Modified /authenticate route logic
  • app/pesu.py – Updated scraping or authentication handling

🧩 Models

  • app/models/request.py – Input validation or request schema changes
  • app/models/response.py – Authentication response formatting
  • app/models/profile.py – Profile extraction logic

🐳 DevOps & Config

  • Dockerfile – Changes to base image or build process
  • .github/workflows/*.yaml – CI/CD pipeline or deployment updates
  • pyproject.toml / requirements.txt – Dependency version changes
  • .pre-commit-config.yaml – Linting or formatting hook changes

📊 Benchmarks & Analysis

  • scripts/benchmark_auth.py – Performance or latency measurement changes
  • scripts/analyze_benchmark.py – Benchmark result analysis changes
  • scripts/run_tests.py – Custom test runner logic or behavior updates

📸 Screenshots / API Demos (if applicable)

Add any visual evidence that supports your changes. MANDATORY for breaking changes.

Examples:

  • Terminal output from a successful curl request (redact sensitive data)
  • Screenshots of Postman/Bruno results
  • GIF of the endpoint working in a browser
  • JSON payloads (redact sensitive data)

🧠 Additional Notes (if applicable)

Use this space to add any final context or implementation caveats.

Examples:

  • Edge cases or limitations to be aware of
  • Follow-up work or tech debt to track
  • Known compatibility issues (e.g., with certain Python versions)
  • Any new issues this PR introduces or makes visible

@niffy-artist2876 niffy-artist2876 requested a review from a team as a code owner September 8, 2025 15:12
Comment on lines 11 to 12
RESULTS_DIR = Path(__file__).parent / "results"
RESULTS_DIR.mkdir(exist_ok=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default path should be benchmark/results.

outfile = (
timestamp = datetime.now().strftime("%Y%m%d_%H%M%S")
if output:
outfile = Path(output)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a check to ensure path to the outfile exists. Make the parent directories if it does not exist.

f"_[execution_mode={'par' if parallel else 'seq'}]"
".csv"
)
outfile = RESULTS_DIR / filename
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can simply compute and use the results dir here, since it is not being used anywhere else. Also, no need to make it a constant.

Comment on lines 147 to 154
"""outfile = (
output
if output
else (
f"benchmark_[num_requests={num_requests}]_[max_workers={max_workers}]_"
f"[parallel={parallel}]_[route={route}]_[timeout={timeout}].csv"
)
)
)"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this docstring?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uhh this was old code, i thought it would be a god idea to keep this as a docstring in case a slip up happens 😅

f.write("status,time\n")
f.writelines(f"{s},{t}\n" for s, t in zip(success, times, strict=False))

print(f"Results saved to the following directory: {outfile}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

outfile is a file, not directory. So update the string to: Results saved to:

Copy link
Member

@aditeyabaral aditeyabaral left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please incorporate similar changes in the unauthenticated_csrf_token_expiry.py file as well.

@achyu-dev
Copy link
Contributor

@niffy-artist2876 any update on this PR?

made all the required changes to benchmark_requests.py, had to delay work on this because of ISA
@niffy-artist2876
Copy link
Author

@niffy-artist2876 any update on this PR?

@niffy-artist2876
Copy link
Author

Please incorporate similar changes in the unauthenticated_csrf_token_expiry.py file as well.

I might have to take some time to understand what the code does before incorporating such changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants