test: Add test for services #1320

NishaSharma14 · 2025-12-17T10:02:44Z

No description provided.

…bsolete SyncProjectAuthorsActionTest

codecov · 2025-12-17T10:07:17Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.55%. Comparing base (6637646) to head (ce0f170).

Additional details and impacted files

@@                Coverage Diff                @@
##             development    #1320      +/-   ##
=================================================
+ Coverage          56.63%   62.55%   +5.92%     
  Complexity          2268     2268              
=================================================
  Files                207      207              
  Lines               8733     8733              
=================================================
+ Hits                4946     5463     +517     
+ Misses              3787     3270     -517

Flag	Coverage Δ
unittests	`62.55% <ø> (+5.92%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot

Pull request overview

This PR reorganizes and expands test coverage for service classes by moving unit tests to feature tests, fixing namespaces, and adding comprehensive test suites for various services.

Key Changes:

Moves unit tests for SyncProjectAuthors and CommonChemistry to feature tests in appropriate locations
Fixes namespaces for ELN-related tests to match their directory structure (Tests\Feature\ExternalServices\ELN)
Adds comprehensive test coverage for new service classes including FileIntegrityService, ELNMetadataServiceFactory, ChemotionMetadataService, DOIService, and others

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 15 comments.

Show a summary per file

File	Description
tests/Unit/SyncProjectAuthorsActionTest.php	Deleted - tests moved to ManageAuthorsTest.php as feature tests
tests/Unit/CommonChemistryTest.php	Deleted - tests moved to CASServiceTest.php as feature tests
tests/Feature/Auth/NFDIAAIProviderTest.php	Deleted - moved to ExternalServices directory with expanded coverage
tests/Feature/ExternalServices/ELN/ProcessDraftELNSubmissionProxyTest.php	Fixed namespace from Tests\Unit to Tests\Feature\ExternalServices\ELN
tests/Feature/ExternalServices/ELN/ELNSubmissionTrackingTest.php	Fixed namespace, unskipped tests, and updated to use Queue faking
tests/Feature/ExternalServices/ELN/ChemotionRepositoryTrackerServiceTest.php	Fixed namespace and added extensive error handling tests
tests/Feature/ManageAuthorsTest.php	Added SyncProjectAuthors action tests moved from unit tests
tests/Feature/FileSystemTest.php	Added comprehensive FileIntegrityService, StorageSignedUrlService, and PathGeneratorService tests
tests/Feature/ExternalServices/NFDIAAIProviderTest.php	New file with comprehensive provider tests including user mapping and authentication
tests/Feature/ExternalServices/ELN/ELNMetadataServiceFactoryTest.php	New file testing factory pattern for ELN metadata services
tests/Feature/ExternalServices/ELN/ChemotionMetadataServiceTest.php	New file with extensive tests for Chemotion metadata extraction and validation
tests/Feature/ExternalServices/DOIServiceTest.php	New file testing DOI service operations with DataCite
tests/Feature/ExternalServices/CASServiceTest.php	Renamed from CASIntegrationTest and added CommonChemistry service tests

Comments suppressed due to low confidence (2)

tests/Feature/ExternalServices/ELN/ELNSubmissionTrackingTest.php:25

The test was previously skipped with the comment "must be revisited." Now it's been unskipped and modified. Consider adding a comment or commit message explaining what was fixed or why it's now safe to run, to provide context for future developers.
tests/Feature/ExternalServices/ELN/ELNSubmissionTrackingTest.php:147
The comment states "The tracking integration in PublishStudy might not be fully implemented yet" which suggests incomplete functionality. If this is the case, consider using markTestIncomplete() or adding a todo comment with a ticket reference to track when the full implementation will be added.

Copilot · 2025-12-17T10:19:05Z

tests/Feature/ManageAuthorsTest.php

+        DB::flushQueryLog();
+        DB::enableQueryLog();
+        $syncProjectAuthors->handle($project, $authorsData);
+        $queries = DB::getQueryLog();
+        $this->assertLessThan(15, count($queries));


Using query log counting to prevent N+1 queries is fragile and can break with framework updates or unrelated changes. Consider using a package like Laravel's built-in query count assertions or a dedicated N+1 detection package. The threshold of 15 queries is also arbitrary and doesn't clearly communicate the expected behavior.

Suggested change

DB::flushQueryLog();

DB::enableQueryLog();

$syncProjectAuthors->handle($project, $authorsData);

$queries = DB::getQueryLog();

$this->assertLessThan(15, count($queries));

$this->expectsDatabaseQueryCount(14);

$syncProjectAuthors->handle($project, $authorsData);

Copilot · 2025-12-17T10:19:06Z

tests/Feature/FileSystemTest.php

        $this->assertEquals('varian', $varianFolder->instrument_type);
    }
+
+    // FileIntegrityService Tests


The comment "FileIntegrityService Tests" indicates a new section, but it's placed in the middle of the FileSystemTest class. Consider organizing these tests into a separate test file (e.g., FileIntegrityServiceTest.php) to follow single responsibility principle and improve test organization.

Suggested change

// FileIntegrityService Tests

// File integrity related tests

Copilot · 2025-12-17T10:19:06Z

tests/Feature/FileSystemTest.php

+
+    public function test_retry_failed_verifications(): void
+    {
+        \Illuminate\Support\Facades\Storage::fake('local');


Using the fully qualified class name for Storage facade is inconsistent with the import at line 13. Use the imported facade class consistently throughout the file instead of the fully qualified name.

Copilot · 2025-12-17T10:19:06Z

tests/Feature/FileSystemTest.php

+            'integrity_status' => 'pending',
+        ]);
+
+        \Illuminate\Support\Facades\Storage::disk('local')->put('drafts/test.txt', $fileContent);


Using the fully qualified class name for Storage facade is inconsistent with the import at line 13. Use the imported facade class consistently throughout the file instead of the fully qualified name.

Copilot · 2025-12-17T10:19:07Z

tests/Feature/FileSystemTest.php

+            'integrity_status' => 'pending',
+        ]);
+
+        \Illuminate\Support\Facades\Storage::disk('local')->put('drafts/test.txt', $fileContent);


Using the fully qualified class name for Storage facade is inconsistent with the import at line 13. Use the imported facade class consistently throughout the file instead of the fully qualified name.

Copilot · 2025-12-17T10:19:08Z

tests/Feature/FileSystemTest.php

+
+    public function test_download_file_from_storage_returns_null_when_missing(): void
+    {
+        \Illuminate\Support\Facades\Storage::fake('local');


Using the fully qualified class name for Storage facade is inconsistent with the import at line 13. Use the imported facade class consistently throughout the file instead of the fully qualified name.

Copilot · 2025-12-17T10:19:08Z

tests/Feature/FileSystemTest.php

+            'verification_attempts' => 1,
+        ]);
+
+        \Illuminate\Support\Facades\Storage::disk('local')->put('drafts/test.txt', $fileContent);


Using the fully qualified class name for Storage facade is inconsistent with the import at line 13. Use the imported facade class consistently throughout the file instead of the fully qualified name.

Copilot · 2025-12-17T10:19:09Z

tests/Feature/ManageAuthorsTest.php

+        DB::shouldReceive('transaction')->once()->andReturnUsing(function ($callback) {
+            return $callback();
+        });


Mocking the DB facade's transaction method in a feature test is an anti-pattern. This test should verify that the transaction behavior works correctly in integration, not mock it. If you need to test transaction behavior, consider using database transactions and verifying rollback on errors, or move this specific test to a unit test where mocking is more appropriate.

Copilot · 2025-12-17T10:19:09Z

tests/Feature/FileSystemTest.php

+
+    public function test_verify_file_integrity_fails_with_mismatched_checksum(): void
+    {
+        \Illuminate\Support\Facades\Storage::fake('local');


Using the fully qualified class name for Storage facade is inconsistent with the import at line 13. Use the imported facade class consistently throughout the file instead of the fully qualified name.

Copilot · 2025-12-17T10:19:09Z

tests/Feature/FileSystemTest.php

+
+    public function test_verify_file_integrity_fails_with_size_mismatch(): void
+    {
+        \Illuminate\Support\Facades\Storage::fake('local');


Using the fully qualified class name for Storage facade is inconsistent with the import at line 13. Use the imported facade class consistently throughout the file instead of the fully qualified name.

NishaSharma14 added 2 commits December 17, 2025 11:00

test: Add tests for external services and reorganize test structure

a96433f

test: improve test for FileSystemTest and ManageAuthorsTest, remove o…

ce0f170

…bsolete SyncProjectAuthorsActionTest

NishaSharma14 requested review from Copilot and vcnainala December 17, 2025 10:02

Copilot started reviewing on behalf of NishaSharma14 December 17, 2025 10:03 View session

NishaSharma14 linked an issue Dec 17, 2025 that may be closed by this pull request

Add test for External Services #1319

Open

Copilot AI reviewed Dec 17, 2025

View reviewed changes

NishaSharma14 self-assigned this Dec 17, 2025

	// FileIntegrityService Tests
	// File integrity related tests

test: Add test for services #1320

Are you sure you want to change the base?

test: Add test for services #1320

Uh oh!

Conversation

NishaSharma14 commented Dec 17, 2025

Uh oh!

codecov bot commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Dec 17, 2025 •

edited

Loading