Skip to content
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

Meta: Docs and Tests for DRACOLoader Update #25478

Closed
wants to merge 1 commit into from
Closed

Meta: Docs and Tests for DRACOLoader Update #25478

wants to merge 1 commit into from

Conversation

epreston
Copy link
Contributor

Related PR: #25475

Description

Update docs and add tests to support the updates to DRACOLoader.

Add DRACOLoader Tests
Update DRACOLoader docs.

DRACOLoader extends from Loader.
Can instantiate a DRACOLoader.
@LeviPesin
Copy link
Contributor

I think it was decided to remove all addons tests until all core classes are fully covered by them.

By the way, can we use import maps in tests?

@epreston
Copy link
Contributor Author

epreston commented Feb 10, 2023

Not sure. There's some very unique views on tests, I have concerns they won't get merged. Such a waste.

No import maps. Be sure to import the sources as the unit does to get clean tests or they will fail. Notice in this one I am importing the way the unit does. loader imported from the source is a different object to loader imported as a three export. Change the comment and try the inheritance test to see it error out.

@LeviPesin
Copy link
Contributor

Related: #23348 #23352

@LeviPesin
Copy link
Contributor

Notice in this one I am importing the way the unit does. loader imported from the source is a different object to loader imported as a three export. Change the commend and try the inheritance test to see it error out.

I think it should work if both are imported from three.

@epreston
Copy link
Contributor Author

epreston commented Feb 10, 2023

The javascript interpreter sees them as different types of objects. What we think doesn't change that. There's articles that go into it if you are interested. How about taking a few seconds to uncomment the line ?

I read those issues, I can see what they were missing. It catches me out too sometimes.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 10, 2023

Please avoid the addition for unit tests for addons. Let's first improve the test coverage for the core.

@Mugen87 Mugen87 closed this Feb 10, 2023
@epreston
Copy link
Contributor Author

I can help with that. Can you merge in my batch of test updates and I will complete the rest for core ?

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