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

Fix : Equation question reference not directly updated #1001

Merged
merged 2 commits into from Feb 16, 2018

Conversation

Shnoulle
Copy link
Collaborator

@Shnoulle Shnoulle commented Feb 8, 2018

  • Fixed issue #13317: Equation question reference not directly updated
  • Move em_javascript to own package (allow more easily inclusion and dependance)
    • Unsure for insideClass var name, if someone have a better idea …
    • but clearly coreClass must wrap answer part (like other item)

…in piped text.

Dev: use trigger, and trigger in em_manager_helper
Dev: choose to hide equation part seems more template related (?)
@Shnoulle
Copy link
Collaborator Author

Shnoulle commented Feb 8, 2018

@olleharstedt since tests is out of git : i can't update it. But test need update here
./assets/packages/expressions/em_javascript.js can do the trick i think.

Sorry , bad eye : done

@olleharstedt
Copy link
Collaborator

olleharstedt commented Feb 9, 2018

Would you mind writing a test for this use-case? I can help you. Maybe write it in pseudo-code first.

@olleharstedt
Copy link
Collaborator

Hm, someone should fix that Travis error first. Yes, it's not related to this commit.

@Shnoulle
Copy link
Collaborator Author

Shnoulle commented Feb 9, 2018

@olleharstedt You need 3 question One to update the equation + equation + one to show equation value. Maybe add an id in the lss ?

<div id="result">{Equation.NAOK}</div>

I give a try when i found times …

@TonisOrmisson
Copy link
Collaborator

@olleharstedt its not only "that" travis error.
Merging master into develop created a bunch of errors. I was starting to address them in the beginning of the week but they just seemed endless :) anyway here's a run with all tests with a fork of current develop branch

https://travis-ci.org/TonisOrmisson/LimeSurvey/jobs/339593111

@olleharstedt
Copy link
Collaborator

Potentially, it looks worse than it is. Some errors will spit out error messages on several or every test, so often, one fix can be enough.

@TonisOrmisson
Copy link
Collaborator

probably looks worse yes. still I fixed i guess 2-3 in one feature branch and I got still problems and gave up eventually :(

Anyway it would be excellent if anybody would look into the test failures. I think they are really valuable and helpful and I personally don't want to do much while the base tests are failing ....

@olleharstedt
Copy link
Collaborator

I agree, but I'm getting increasingly tired of fixing other people's bugs. The one who broke it first should fix it. It should be visible in the Travis build history.

@TonisOrmisson
Copy link
Collaborator

I also agree, but this started with master merged into develop and this is kind of a special case.
It started here - the merge:
https://travis-ci.org/LimeSurvey/LimeSurvey/builds/337480044

I think fixing the test failures after merge should be a team effort anyway.

@TonisOrmisson
Copy link
Collaborator

Also. It seems we need a bit more isolation between the tests. It should not be that one failing test will make most of the other tests fail also. Eg currently the installation test failing causes (i think) a lot of othr tests fail etc. They should be more isolated

@TonisOrmisson
Copy link
Collaborator

but to conclude on a positive note. I am still very happy that we have the tests finally :)

@olleharstedt
Copy link
Collaborator

Hm, yeah, I guess merge problems are a slightly different beast.

Isolation between tests is a reoccurring issue. It's fairly good right now, but yes, the installation test can definitely fail better.

If many tests fail, it's usually because of a bug in the code which affects all of those tests (syntax error being the most obvious example).

@TonisOrmisson
Copy link
Collaborator

turns out it was "that" one fix afterall
here you go #1003

@Shnoulle Shnoulle merged commit a4ad787 into LimeSurvey:develop Feb 16, 2018
@Shnoulle Shnoulle deleted the develop_13317 branch May 4, 2022 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants