Skip to content

Implement a syntactic workspace-wide test index #1175

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 24, 2024

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Apr 15, 2024

This workspace-wide syntactic test index is used for two purposes:

  • It is used for XCTests instead of the semantic index for files that have on-disk or in-memory modifications to files
  • It is uses for swift-testing tests, which are only discovered syntactically

rdar://119191037

@ahoppen ahoppen requested a review from benlangmuir as a code owner April 15, 2024 21:53
@ahoppen ahoppen marked this pull request as draft April 15, 2024 21:53
@ahoppen ahoppen changed the title Implement a syntactic workspace-wide test index 🚥 $1151 Implement a syntactic workspace-wide test index 🚥 #1151 Apr 15, 2024
@ahoppen ahoppen force-pushed the syntactic-test-index branch from 788a750 to 256a524 Compare April 16, 2024 00:05
@ahoppen ahoppen force-pushed the syntactic-test-index branch from 256a524 to 4bcfd11 Compare April 18, 2024 21:44
@ahoppen ahoppen changed the title Implement a syntactic workspace-wide test index 🚥 #1151 Implement a syntactic workspace-wide test index Apr 18, 2024
@ahoppen ahoppen requested a review from bnbarham April 18, 2024 21:44
@ahoppen ahoppen marked this pull request as ready for review April 18, 2024 21:44
@ahoppen
Copy link
Member Author

ahoppen commented Apr 18, 2024

CC @grynspan @stmontgomery

@ahoppen ahoppen force-pushed the syntactic-test-index branch from 4bcfd11 to c7f6e5f Compare April 18, 2024 21:46
@ahoppen
Copy link
Member Author

ahoppen commented Apr 18, 2024

@swift-ci Please test

@grynspan
Copy link

Is there anything specific you'd like us to look at here?

@ahoppen
Copy link
Member Author

ahoppen commented Apr 18, 2024

I don’t think there’s anything in particular. Mostly wanted to make you aware of it in case anything sticks out to you.

@ahoppen
Copy link
Member Author

ahoppen commented Apr 18, 2024

@swift-ci Please test Windows

@ahoppen ahoppen force-pushed the syntactic-test-index branch from c7f6e5f to 99824cf Compare April 19, 2024 03:53
@ahoppen
Copy link
Member Author

ahoppen commented Apr 19, 2024

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Apr 19, 2024

@swift-ci Please test Windows

case .changed:
rescanFiles([fileEvent.uri])
case .deleted:
removeFilesFromIndex([fileEvent.uri])
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't this end up cancelling a whole batch that contains this file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, good catch. I didn’t think about that when changing to batch indexing.

Modified it to explicitly keep track of removed files instead of relying on task cancellation.

Comment on lines +156 to +157
// - For all files that have been not been modified since they were last indexed in the semantic index, include
// XCTests from the semantic index.
Copy link
Contributor

Choose a reason for hiding this comment

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

I just checked and the index does indeed contain a relationship from the macro reference to the attached declaration. So it seems we could pretty easily add semantic index results for swift-testing by eg. finding references to Suite and Test, then grabbing their contained by relationship.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, nice. Let’s do that in a follow-up PR. rdar://126721322 is tracking that.

// - All files that have in-memory modifications are syntactically scanned for tests here.
var outOfDateChecker = IndexOutOfDateChecker(documentManager: documentManager)

let filesWithInMemoryState = documentManager.documents.compactMap { (uri, document) -> DocumentURI? in
Copy link
Contributor

Choose a reason for hiding this comment

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

Could expose hasFileInMemoryModifications from IndexOutOfDateChecker and then use it instead of the check here?

Writing that out also made me notice that hasFileInMemoryModifications reads a bit weirdly, maybe fileHasInMemoryModifications? Or just hasInMemoryModifications since it takes a URL parameter anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. That way we can re-use the cache of the check as well.

ahoppen added 2 commits April 23, 2024 09:25
This workspace-wide syntactic test index is used for two purposes:
- It is used for XCTests instead of the semantic index for files that have on-disk or in-memory modifications to files
- It is uses for swift-testing tests, which are only discovered syntactically.

rdar://119191037
@ahoppen ahoppen force-pushed the syntactic-test-index branch from 99824cf to b3c519b Compare April 23, 2024 16:25
@ahoppen
Copy link
Member Author

ahoppen commented Apr 23, 2024

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Apr 23, 2024

@swift-ci Please test Windows

let removedFiles = Set(self.indexedTests.keys).subtracting(testFiles)
removeFilesFromIndex(removedFiles)

rescanFiles(testFiles)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should rescanning reset removed files? If a file was re-added (say switching branches), seems like we'd forever ignore it right now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, very good catch. Fixed it and added a test for it.

@ahoppen ahoppen force-pushed the syntactic-test-index branch from 3b211b2 to 420d2bf Compare April 24, 2024 03:50
@ahoppen ahoppen force-pushed the syntactic-test-index branch from 420d2bf to ae215f9 Compare April 24, 2024 03:51
@ahoppen
Copy link
Member Author

ahoppen commented Apr 24, 2024

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Apr 24, 2024

@swift-ci Please test Windows

1 similar comment
@ahoppen
Copy link
Member Author

ahoppen commented Apr 24, 2024

@swift-ci Please test Windows

@ahoppen ahoppen force-pushed the syntactic-test-index branch from abf31a8 to ae215f9 Compare April 24, 2024 05:18
@ahoppen ahoppen merged commit ec5c614 into swiftlang:main Apr 24, 2024
3 checks passed
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